Fix/minute division

[jspaezp]

Hello there!

I noticed that the behavior of some libs was inconsistent.

Before the patch (03b03f1)

# mzdata::/src/spectrum/scan_properties.rs:138 says the units should be minutes
<Spectrum=1>
[1]MS:1000894|retention time=1719.509460 # -> is returned as 1719.509460 (seconds)
[1]UO:0000000|unit=UO:0000010|second
<Peaks>

<Spectrum=2>
[1]MS:1000894|retention time=28.658491 # -> is returned as 0.4776415189107259 (minutes / 60)
[1]UO:0000000|unit=UO:0000031|minute
<Peaks>

After this both options are returned as 28.658491

also I left a note on the test LMK what you think ...

[jspaezp]

Note: I am struggling a bit on how to represent this in my libraries ... if my library is in "biognosys iRT scale"... it feels wrong to set minutes, when the values can be negative.

[jspaezp]

Note: This "sentinel value conditional" could introduce a bug if a library has both indexed and non-indexed retention times. since the indexed time CAN have values of 0.0.

Imagine spectrum 1 defines iRT= -5, RT = 5 -> assigns -5 (bc the second pass does not over-write the 0.0) as the RT. then spectrum 2 defined iRT=0.0 and RT = 10 -> assigns 10 (because the second pass does over-write it)

[mobiusklein]

The bit about the value being stored in minutes is my intent, I don't have any type guards in place for it. I could use uom as Douwe does in mzcore, but at the time I was designing the API, it wasn't ergonomic. Admittedly that was also when I was learning Rust. The code building the ScanEvent instance would need to explicitly convert the time unit.

The iRT term probably shouldn't be stored as ScanEvent's native time attribute. iRT as a concept is calibrated on whatever unit you fit the iRT regression, and shouldn't be converted for consistency. Otherwise you have to update your coefficients and things get even messier. Although, this raises another question since iRT-calibrated RT predictors like Prosit are effectively producing predictions on that scale, and you do a post-hoc recalibration of your data to the predicted scale. In that case you could argue that as long as the model is consistent, and you're still doing the post-hoc transform.

I was going to argue that "sure time can be negative as long as it is effectively linear sequential", but odds are most code assumes that time starts at 0 rather than checking the minimum value. This is mostly because we assume we are reading out spectra in time-order from most run file formats, except maybe MGF.

I'll try to find time to do an API review again later this coming week, but I'll be traveling for most of it.

[jspaezp]

Enjoy your travels! thanks for taking a look at it!

(just so we are on the same page ... the issue here is that the factor logic is flipped ... so 28.xxx minutes gets divided by 60 instead of being left alone, and as a counterpart 1719.509460 seconds gets assigned as 1719.509460 to a "minutes" field).

[douweschulte]

This definitively is a bug right now so thanks for opening the PR. I must admit that I assumed mzdata would work in seconds. This might be a bit too much side information for this PR but I have seen this field populated with seconds and with minutes when parsing different raw file formats. Now that I know it is supposed to be minutes I will remember when it is not correct and fix it in mzdata (or bother Joshua :-) ).

Two small questions though, why change the group to 2 or more members? Did you see any file with more then 2 members in a unit group? If so and if you are allowed to share I would like to see that as an example to make sure that all other group detections are updated if necessary. The other question is why introduce the black_box()? That function is supposed to prevent to compiler from optimising the code that is passed in there for correctness of benchmarks. I think adding a warning makes most sense, but I see that that is not set up yet in the function, so feel free to leave TODO: set up warning or something similar than I will get to that next time I touch code in that area.

Once these questions are answered I will merge the PR.

[jspaezp]

@douweschulte

why change the group to 2 or more members?

I was thinking .. . If I have an element annotated like so:

<Spectrum=5>
[1]MS:1000894|retention time=1719.509460
[1]UO:0000000|unit=UO:0000010|second
[1]MS:1003174|attribute maximum=1800.0
[1]MS:1003175|attribute minimum=1600.0
<Peaks>

the "old" == 2 would just skip the block and report the time as 0.

(LMK if that is not the right way to annotate it ... I am just getting to know the specification, it was a derivation from https://github.com/HUPO-PSI/mzSpecLib/blob/506791706f41e95e0c2a1d4de15e49f25b59c760/examples/SpectraST/fetal_brain_tiny_consensus_td.mzSpecLib.txt#L433 )

black_box(())

It was just a hack to silence a clippy warning because two branches have the same code. (and also reminds me that I would suggest some form of warning mechanism on that line to tell the user "we are writing this as minutes but we are not sure what unit it actually has" ...)

I am happy to change it LMK