Keep MS:1000516 charge array aligned through reader reorder/trim points

[pcarvalho75]

What

Follow-up to #1060. Fix the chargeArray permutation-sync gap @trishorts identified in review: the per-peak charge array was being loaded in source order and passed straight to the MsDataScan constructor, but the reader paths reorder/trim mzs and intensities in three places without touching chargeArray:

1. Zero-intensity peak removal (Array.Sort(intensities, mzs) + SubArray + Array.Sort(mzs, intensities)) in both GetMsDataOneBasedScanFromConnection (static) and GetOneBasedScanFromDynamicConnection (dynamic).
2. Optional WindowModeHelper.Run pruning when FilteringParams is set.
3. The unconditional final Array.Sort(masses, intensities) at the end of each path.

Why

Without this fix, charges silently misalign with their peaks whenever the source mzML has any peak with intensity below 0.01, any out-of-order m/z, or filterParams is non-null. Lengths can still match while values point at the wrong peaks, so the reader's existing length-only policy can't catch it. Self-roundtrip tests with pre-sorted, non-zero-intensity, no-filter data wouldn't expose it — exactly how it slipped past #1060's tests.

The motivating consumers (YADA, mzR, pymzML) trust that ChargeArray[i] describes the peak at MassSpectrum.XArray[i]. Without the fix, that contract holds in source order but breaks the moment the reader reorders anything.

Implementation

WindowModeHelper.cs

New charge-aware overload Run(ref intensities, ref mArray, ref chargeArray, ...). The existing 5-parameter signature is preserved as a thin wrapper that delegates with chargeArray = null, so the eight other callers (MzSpectrum, TofSpectraMerger, ThermoRawFileReader, Mgf, Bruker, plus the four existing tests) compile and behave unchanged.

When chargeArray is non-null and length-matches mArray, the new overload carries it through:

• The intensity-sort at the top (3-way index permutation; locals copied because C# can't capture ref parameters in lambdas).
• The per-window selection — mzInRange[rangeIndex] already stores indices into the sorted arrays, so charges follow trivially via chargeArray[arrayIndex].
• The final m/z-sort (same 3-way index-permutation idiom).

Length-mismatched arrays are dropped — preserving an ambiguous one would compound the corruption this PR is meant to prevent.

Mzml.cs reader paths

Both GetMsDataOneBasedScanFromConnection and GetOneBasedScanFromDynamicConnection now use a small private SortInPlaceByKey(double[] keys, double[] valuesA, int[] valuesB) helper for the 3-array sorts. Each reorder/trim point gets the charge-aware branch when chargeArray is non-null and length-aligned, and falls back to the original 2-array Array.Sort when there's no charge data. The new WindowModeHelper.Run overload receives chargeArray directly.

Defensive policy: any time the length invariant would be violated, chargeArray is set to null and the MsDataScan is constructed without it. That's better than silently shipping misaligned charges.

Tests

Three new regression tests in TestMzML.cs, one per reorder/trim point:

Test	Targets
ChargeArrayUnsortedMzInputAlignsAfterReaderSort	The final Array.Sort(masses, intensities). Constructs an MzSpectrum with mzs in reverse order via the shouldCopy:false ctor (writer emits unsorted bytes verbatim — MzSpectrum.Get64BitXarray does no sort), asserts the post-sort (mz, charge) pairing matches the source pairing peak-for-peak.
ChargeArraySurvivesZeroIntensityFilter	The zero-intensity removal block. Source includes a peak with intensity 0.005, charge 9. After the reader's trim, asserts only the >0.01 peaks survive AND charges align with the surviving (mz, intensity) pairs (the dropped charge is the one paired with the dropped intensity, not just "the first one").
ChargeArraySurvivesFilteringParamsPruning	The WindowModeHelper.Run path. 6 peaks, FilteringParams { numberOfPeaksToKeepPerWindow: 3, numberOfWindows: 1, applyTrimmingToMs1: true }. Asserts the 3 most-intense peaks survive AND their charges still line up with their original mz values after the intensity-sort, per-window top-N selection, and final m/z-sort.

Passed ChargeArrayDefaultsToNullWhenAbsent     (existing — 2-array baseline branch)
Passed ChargeArrayRoundTrip                     (existing)
Passed ChargeArrayRoundTripWithNoiseData        (existing — 6-array branch)
Passed ChargeArrayUnsortedMzInputAlignsAfterReaderSort
Passed ChargeArraySurvivesZeroIntensityFilter
Passed ChargeArraySurvivesFilteringParamsPruning

Broader regression: 103/103 in the TestMzML + WindowMode + SpectrumProcessing + Dynamic sweep.

Backward compatibility

Strictly additive on the public API:

• WindowModeHelper.Run — old 5-parameter signature unchanged; new 6-parameter overload added alongside.
• Mzml.cs — both reader paths produce identical output for any mzML without an MS:1000516 charge array (the hasCharges == false branches are byte-equivalent to the pre-PR code).
• MsDataScan.ChargeArray — public type and semantics unchanged.

Files changed

File	Change
mzLib/MassSpectrometry/WindowModeHelper.cs	Added charge-aware Run overload; existing signature delegates.
mzLib/Readers/MzML/Mzml.cs	Both reader paths apply the same permutation/mask to chargeArray everywhere they touch m/z or intensities; new private SortInPlaceByKey helper for 3-array sorting.
mzLib/Test/FileReadingTests/SpectraFileReading/TestMzML.cs	Three new regression tests covering each reorder/trim point.

cc @trishorts @nbollis — addresses the follow-up issue from the PR #1060 review thread.

[codecov]

Codecov Report

❌ Patch coverage is 84.05797% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.45%. Comparing base (646e218) to head (35f43f3).

Files with missing lines	Patch %	Lines
mzLib/Readers/MzML/Mzml.cs	74.28%	12 Missing and 6 partials ⚠️
mzLib/MassSpectrometry/WindowModeHelper.cs	94.11%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #1062    +/-   ##
========================================
  Coverage   81.45%   81.45%            
========================================
  Files         366      366            
  Lines       46899    47018   +119     
  Branches     5556     5579    +23     
========================================
+ Hits        38200    38298    +98     
- Misses       7620     7634    +14     
- Partials     1079     1086     +7     

Files with missing lines	Coverage Δ
mzLib/MassSpectrometry/WindowModeHelper.cs	97.44% <94.11%> (-1.79%)	⬇️
mzLib/Readers/MzML/Mzml.cs	85.05% <74.28%> (-0.98%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pcarvalho75]

Heads-up: while finalizing this PR we kept digging and realized there is a deeper issue underneath what @trishorts caught. I have opened #1063 as a superseding PR that addresses the root cause; this one is still valid as-is if you would prefer to land the smaller fix first, and we can close this one in favor of #1063 whenever you decide.

Wanted to lay out what slipped by us all in #1060 + this PR, since the same bug class exists in two more places and the structurally-correct fix is to move ChargeArray so the type system makes the bug class impossible.

What this PR does (#1062)

Patches three reorder/trim points across two reader paths so chargeArray follows the same permutation/mask as m/z and intensities through:

• zero-intensity removal,
• WindowModeHelper.Run filtering,
• the unconditional final Array.Sort(masses, intensities).

That is six fix sites total, plus a new charge-aware overload on WindowModeHelper.Run and three regression tests. All of that is still correct and minimal. Nothing in here is wrong.

What we missed (and what #1063 fixes)

The bug class is broader than the reader paths. As long as ChargeArray lives on MsDataScan while the peaks it indexes (XArray, YArray) live on MsDataScan.MassSpectrum (an MzSpectrum), every MzSpectrum mutator silently invalidates the alignment invariant - because MzSpectrum does not know it has a parent MsDataScan, so it cannot reach back to update chargeArray even if it wanted to. Two concrete instances we verified in the current code:

1. MzSpectrum.XCorrPrePreprocessing (lines 509-605 of MzSpectrum.cs)

Array.Sort(XArray, YArray);                         // 1: re-sort by m/z
// ...
WindowModeHelper.Run(ref intensities, ref masses,   // 2: window-prune
    secondFilter, ...);                             //    (no charge awareness)
// ...
YArray = intensites.ToArray();                      // 3: rebuild to a NEW length
XArray = masses.ToArray();                          //    (nominal-mass bins)
Array.Sort(this.XArray, this.YArray);               // 4: final re-sort

After this method runs, the parent scan's MsDataScan.ChargeArray:

• has the wrong length (still pointing at the pre-XCorr peak count, even though the spectrum has been re-binned to a different number of nominal-mass bins),
• and is indexed against peaks that no longer exist or are in a totally different order.

The reader-side fix in this PR does not help here at all - this is a downstream mutation that happens after the reader has long returned. Any pipeline that calls XCorr (search engines doing classic Sequest scoring, for example) on a scan whose mzML included an MS:1000516 charge array would have silently corrupted charges in its working copy of that spectrum.

2. MsDataScan.TransformMzs / MzSpectrum.ReplaceXbyApplyingFunction

Calibration. Applies an arbitrary Func<MzPeak, double> to every m/z. Affine calibration (mz' = a*mz + b) preserves order so charges remain valid. Non-monotonic calibration (segmented, interpolated lock-mass, EThcD energy-corrected) can flip the order, leaving the parent's chargeArray pointing at the wrong peaks if any subsequent code re-sorts.

ReplaceXbyApplyingFunction itself does not re-sort, so charges remain index-aligned right after calibration. The risk is purely: if any later code does Array.Sort(spectrum.XArray, spectrum.YArray) to canonicalize, alignment breaks silently. With ChargeArray on MsDataScan that is invisible. With ChargeArray on MzSpectrum, the same Array.Sort is a code smell that will be caught in review (or replaced by a charge-aware sort helper).

How #1063 fixes it

Moves ChargeArray onto MzSpectrum (parallel to XArray/YArray, where it belongs structurally), adds a length-validating 4-arg ctor on MzSpectrum, makes MsDataScan.ChargeArray a delegating property over MassSpectrum.ChargeArray, makes XCorrPrePreprocessing explicitly drop ChargeArray (since the output peaks are nominal-mass bins for which a per-peak charge is undefined), and includes the reader-side fixes from this PR. Strictly additive on the public API - every existing caller compiles unchanged.

The defining win: it becomes structurally impossible to construct an MzSpectrum with a length-mismatched chargeArray. The 4-arg ctor throws at construction time. That is the only reliable defense - a length-only check at use sites cannot catch lengths that happen to match while values point at the wrong peaks (which is exactly the bug @trishorts caught).

11 tests in #1063 cover: round-trip + null-default + 6-array branch (these 3 from #1060), reader-side permutation sync (the 3 from this PR), plus 5 new ones for the architectural change (length-mismatch ctor throw, delegation, conflict detection, backward-compat wrap, XCorr drop).

Side findings (out of scope for either PR)

While digging into all this we noticed two adjacent items, neither in scope of either PR but worth mentioning so they do not slip:

1. FilteringParams.MinimumAllowedIntensityRatioToBasePeakM has a stray trailing M on the public property name. The constructor parameter is minimumAllowedIntensityRatioToBasePeak (no M). Smells like a half-done refactor and produced a CS1739 while we were writing the FilteringParams test for this PR. Cheap fix: rename property + [Obsolete] shim on the old name.

2. MsDataScan has no IEquatable<MsDataScan> today (verified - unrelated to Param equality #1055 which only added it to *Params types). Not a bug, just a heads-up: if anyone adds equality later, ChargeArray belongs in the comparison since two scans with the same peaks but different charges are not semantically equal.

Suggested merge path

Whichever you prefer:

• Merge Move MS:1000516 ChargeArray onto MzSpectrum where the peaks live (supersedes #1062) #1063 as the complete fix, close this PR. Cleanest end state.
• Merge this PR (Keep MS:1000516 charge array aligned through reader reorder/trim points #1062) first as the immediate reader fix, then merge Move MS:1000516 ChargeArray onto MzSpectrum where the peaks live (supersedes #1062) #1063 on top for the architectural improvement. Move MS:1000516 ChargeArray onto MzSpectrum where the peaks live (supersedes #1062) #1063's commit applies cleanly on either base.
• Merge only Keep MS:1000516 charge array aligned through reader reorder/trim points #1062, treat Move MS:1000516 ChargeArray onto MzSpectrum where the peaks live (supersedes #1062) #1063 as a discussion / future direction. We will keep our YADA fork on Keep MS:1000516 charge array aligned through reader reorder/trim points #1062 + the relevant pieces of Move MS:1000516 ChargeArray onto MzSpectrum where the peaks live (supersedes #1062) #1063 in that case.

Apologies for the back-and-forth on this one. The architectural realization came late. Happy to iterate on either PR based on your preference.

cc @trishorts @nbollis

[nbollis]

@pcarvalho75 I think the best approach will be to make one Pull request that fixes the issue. That way you do not need to go through the review process with us twice. If #1063 resolves the issues here in #1062 as well, then I recommend reviewing the failing tests in 1063 and have a single PR to resolve these issues.

[trishorts]

also note that there are labels. if you can, select the "ready for review" label to make sure that we focus on it. I really like the verbose comments in the PR. that makes my reviewing a much easier task. be sure to include xml documentation in the code for key aspects so that we can understand and respect the contract

[pcarvalho75]

Hi. I might be able to only submit the fix next week as I'm in an international trip. But I will definitely submit as soon as I can. Cheers Paulo

…

On Tue, May 12, 2026, 21:10 trishorts ***@***.***> wrote: *trishorts* left a comment (smith-chem-wisc/mzLib#1062) <#1062 (comment)> also note that there are labels. if you can, select the "ready for review" label to make sure that we focus on it. I really like the verbose comments in the PR. that makes my reviewing a much easier task. be sure to include xml documentation in the code for key aspects so that we can understand and respect the contract — Reply to this email directly, view it on GitHub <#1062 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4SFLWV7RBJGXFLZCV4N5T42O4OTAVCNFSM6AAAAACY2AICXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DIMZVHAZTOMRZGI> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[pcarvalho75]

Closing — superseded by #1063 per @nbollis's consolidation request. #1063 already absorbs the reorder/trim sync work from this PR (charge-aware WindowModeHelper.Run overload + SortInPlaceByKey helper in Mzml.cs) and adds the architectural fix moving ChargeArray to MzSpectrum so length mismatches become structurally impossible.