Skip to content

Conversation

@wolfgangcolsman
Copy link
Contributor

The command-line options have been inconsistent

zhaoheting-zontal and others added 30 commits February 24, 2023 01:07
ZSPACE-12467 Check if the file is MS type
ZSPACE-12730 Try to close the underlying stream.
# Conflicts:
#	RawFileParser.cs
#	Writer/MetadataWriter.cs
#	Writer/MzMlSpectrumWriter.cs
@ypriverol ypriverol requested review from caetera and ypriverol March 22, 2024 14:44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change cannot be accepted, see #144

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wolfgangcolsman Could you, please, comment on what was the reason for implementing this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly the implemented changes, the only difference is the selection of the MS device. However, if the RAW file does not contain MS data, generating MGF is impossible. We check for the presence of MS data earlier, thus, we should never come to this point if there was no MS data, or it was not selected before.

Is there an example when this assumption does not hold?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as MGFParser, there is a guard upstream, thus, we should not arrive in WritePScans if the MS device is not selected already.

}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSAnalog chromatograms are taken care of at the same time as Analog ones.
Was the PR against the latest commit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of changes are coming from commits before v 1.4.3 and, likely, were taken care of already.
I will suggest keeping our version and "cherry-picking" only necessary changes.
@wolfgangcolsman could you summarize what was broken?

@caetera caetera mentioned this pull request Apr 8, 2024
@caetera caetera mentioned this pull request Apr 18, 2024
@ypriverol ypriverol merged commit da4b53b into CompOmics:master May 9, 2024
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.

5 participants