Skip to content

fix(PD-converter): Enable spectrumFile to be used for run ID in PD converter #101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

tonywu1999
Copy link
Contributor

@tonywu1999 tonywu1999 commented Nov 1, 2024

Motivation and Context

Similar to this commit, but I'm pretty sure we should use SpectrumFile for the regular MSstats converter. Unit tests for MSstatsPTM seem to assume we use SpectrumFile.

Changes

  • Updated clean function to use SpectrumFile for the regular MSstats converter for PD.

Testing

  • Unit tests pass in MSstatsConvert, MSstats, MSstatsPTM. Previously, tests in MSstatsPTM were failing.
  • Note: I'm a little concerned about backwards compatibility here. People previously using FileID will end up having workflows breaking.
  • Future: I'll add a run column for the PD MSstats and TMT converters to enable run ID.

Checklist Before Requesting a Review

  • I have read the MSstats contributing guidelines
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@mstaniak
Copy link
Contributor

How were people using File.ID? Wasn't Spectrum.File the only working option? If there were actually users who used File.ID, let's add this to news so it's clear that this default changed

@tonywu1999
Copy link
Contributor Author

@mstaniak

Sorry for the confusion - I read this issue and realized that nobody was using file ID. I'll make a note to update the converters themselves to include a run column

@tonywu1999 tonywu1999 merged commit 8f0d7f6 into devel Nov 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants