fix parsing precursor charge in MGF files

[paretje]

This fixes two issues:

• the charge value in the PEPMASS header (can) have the same suffixed format as the CHARGE header
• the order of the headers isn't determined, so the charge value should still be saved correctly if the CHARGE header is before the PEPMASS header

NOTE: There could be multiple PEPMASS headers, and this is supported by the parser, but it's not clear to me how the CHARGE header should behave then: should the charge be set as part of the PEPMASS header then, should there be multiple CHARGE headers, or should there only be one CHARGE header, that would apply to all precursor ions. At the moment, I kept the current behaviour of just setting the last precursor ion, but it might make sense to make this a for loop, keeping behaviour consistent, at least assuming there can only be one CHARGE header. But I might be overthinking what seems to be an edge-case ...

[mobiusklein]

Thank you for adding this.

I'll review this in more detail soon, but can you please provide a toy example MGF file that would fail to parse properly without this change? The MGF parser was one of first things I wrote when I was learning Rust, so the state machine is convoluted and it would be easy to re-break things.

[paretje]

I added the test case that tripped in ms2rescore-rs to test/data/small.mgf and updated the assertions accordingly.

[mobiusklein]

Thank you.

The control flow looks like it handles the two cases correctly now that I've had time to read it.

While you're right that this code won't handle an MGF which includes both a CHARGE header and a charge in the PEPMASS line or an MGF with multiple CHARGE headers, they are less common today thanks to high resolution MS1 spectra. I don't see a need to rework mzdata's data model to support tracking that explicitly.

If you think it needs to be captured, we can devolve secondary charge states to Param using MS:1000633|possible charge state. That'd be purely for the purposes of tracking that information, it'd need explicit help being round-tripped. Otherwise, I can add a warning if we see multiple charge states, but otherwise do nothing with the second charge state unless a user.

[mobiusklein]

If you're happy with the state the PR is in now, please let me know and I'll merge it and cut a new release.

[paretje]

Yes, I'm happy with the state of the PR, I was just pondering about the theoretical correctness of my changes, also given that it doesn't seem that well defined as a standard. I looked at the pyteomics and OpenMS parser for reference, and both actually just assume there is only one precursor.

As I said, this could be change that might be worthwhile to consider, as it seems somewhat more consistent in behaviour:

--- a/src/io/mgf.rs
+++ b/src/io/mgf.rs
@@ -358,13 +358,7 @@ impl<R: io::Read, C: CentroidPeakAdapting, D: DeconvolutedPeakAdapting> MGFReade
                 }
                 "CHARGE" => {
                     builder.precursor_charge = self.parse_charge(value);
-                    if let Some(ion) = builder
-                        .description
-                        .precursor
-                        .get_or_insert_with(Precursor::default)
-                        .iter_mut()
-                        .last()
-                    {
+                    for ion in builder.description.precursor.get_or_insert_with(Precursor::default).iter_mut() {
                         if ion.charge.is_none() {
                             ion.charge = builder.precursor_charge
                         }