Move MS:1000516 ChargeArray onto MzSpectrum where the peaks live (supersedes #1062)

[pcarvalho75]

TL;DR

This supersedes #1062. After working on the reader-side fix from #1062 we realized there is a deeper issue neither the original PR (#1060) nor that follow-up address: as long as ChargeArray lives on MsDataScan while the peaks it indexes live on MsDataScan.MassSpectrum (an MzSpectrum), any MzSpectrum mutator silently invalidates the alignment invariant. This PR moves ChargeArray onto MzSpectrum so charges live with the peak data they describe and every mutator can keep all three arrays in lockstep automatically.

If you would prefer to land the smaller reader-side fix first, #1062 is still ready. This PR includes those reader-side fixes too, so merging this one and closing #1062 also works.

cc @nbollis @trishorts

What we found while working on #1062

@trishorts caught the symptom in the readers: three reorder/trim points (zero-intensity removal, WindowModeHelper, the unconditional final Array.Sort) reordered m/z and intensity but left chargeArray in source order. #1062 patches all six fix sites across the static and dynamic reader paths.

But the same bug class exists at the MzSpectrum layer, where the readers cannot reach:

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 on a scan whose MsDataScan.ChargeArray is non-null, the parent scan's chargeArray:

• has the wrong length (still pointing at the pre-XCorr peak count),
• and is indexed against peaks that no longer exist or are in a totally different order.

MzSpectrum does not know its parent MsDataScan exists - it cannot reach back to update chargeArray. The bug is invisible at every call site.

MsDataScan.TransformMzs / MzSpectrum.ReplaceXbyApplyingFunction

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

The fundamental issue: chargeArray is on the wrong type. It indexes MzSpectrum.XArray/YArray but lives on MsDataScan, and the type system cannot help.

What this PR does

Move ChargeArray onto MzSpectrum

public class MzSpectrum
{
    public double[] XArray { get; private set; }
    public double[] YArray { get; private set; }
    public int[] ChargeArray { get; private set; }   // <- new, parallel to XArray/YArray

    // New 4-arg ctor - validates length-match, throws ArgumentException on mismatch
    public MzSpectrum(double[] mz, double[] intensities, int[] chargeArray, bool shouldCopy);

    // Existing 3-arg ctor delegates with chargeArray=null. Source-compatible.
    public MzSpectrum(double[] mz, double[] intensities, bool shouldCopy);
}

The 4-arg ctor's length validation is the structural defense against the entire bug class. Without it, callers can construct length-mismatched spectra that silently misalign - a length-only check at use sites cannot catch lengths that happen to match while values point at the wrong peaks. Constructor-level enforcement makes it impossible.

MzSpectrum.XCorrPrePreprocessing now drops ChargeArray explicitly

The output peaks are nominal-mass bins (any number of source peaks may collapse into one bin; bins with no source peaks are zero-intensity slots that have no charge to report). A per-peak charge in that representation is undefined, so the honest behavior is to clear ChargeArray rather than ship a broken one. Pinned with MzSpectrumXCorrPreprocessingDropsChargeArray.

MsDataScan.ChargeArray becomes a delegating property

public int[] ChargeArray => MassSpectrum?.ChargeArray;

The legacy chargeArray ctor parameter is preserved for source compatibility:

• If the caller passes it AND the MzSpectrum already carries charges, the ctor requires them to be SequenceEqual and throws ArgumentException on mismatch (programming error - surface immediately rather than silently picking one).
• If only the legacy parameter is supplied, the ctor wraps the bare MzSpectrum into a new charge-carrying one.

Pinned with MsDataScanCtorChargeArrayConflictThrows and MsDataScanCtorWrapsChargesIntoBareMassSpectrumForBackwardCompat.

Reader paths use the new construction (and incorporate #1062's fixes)

Both Mzml.cs reader paths now:

1. Apply the same permutation/mask to chargeArray everywhere they touch m/z or intensities (small private SortInPlaceByKey helper for 3-array sorts; WindowModeHelper.Run gets a charge-aware overload).
2. Construct the MzSpectrum with chargeArray attached via the new 4-arg ctor - instead of passing it separately to the MsDataScan ctor.

This means the reader-side fixes from #1062 are included here. If #1062 lands first that is fine; this PR's commit applies cleanly on top of either base.

WindowModeHelper.Run charge-aware overload

public static void Run(ref double[] intensities, ref double[] mArray, ref int[] chargeArray,
    IFilteringParams filteringParams, double scanRangeMinMz, double scanRangeMaxMz,
    bool keepZeroPeaks = false);

When chargeArray is non-null and length-matches mArray, the overload carries it through the intensity-sort, per-window selection, and final m/z-sort via index permutation. Existing 5-parameter signature preserved as a thin wrapper that delegates with chargeArray=null - every existing caller (8 sites across MzSpectrum, TofSpectraMerger, ThermoRawFileReader, Mgf, Bruker, plus tests) is untouched.

Tests (11 total, all passing)

Test	Targets
ChargeArrayDefaultsToNullWhenAbsent	(existing) Baseline 2-array branch
ChargeArrayRoundTrip	(existing) Charge round-trip + m/z + intensity assertions
ChargeArrayRoundTripWithNoiseData	(existing) 6-array branch (NoiseData + Charge)
ChargeArrayUnsortedMzInputAlignsAfterReaderSort	(was #1062) Reader's final Array.Sort permutes charges in lockstep
ChargeArraySurvivesZeroIntensityFilter	(was #1062) Reader's zero-intensity trim keeps charges aligned
ChargeArraySurvivesFilteringParamsPruning	(was #1062) WindowModeHelper charge-aware path
MzSpectrumChargeArrayLengthMismatchThrows	(new) 4-arg ctor structural defense
MsDataScanChargeArrayDelegatesToMassSpectrum	(new) Delegating property contract
MsDataScanCtorChargeArrayConflictThrows	(new) Backward-compat conflict detection
MsDataScanCtorWrapsChargesIntoBareMassSpectrumForBackwardCompat	(new) Legacy ctor parameter still works
MzSpectrumXCorrPreprocessingDropsChargeArray	(new) Pin the explicit "drop on incompatible mutation" behavior

Broader regression sweep (TestMzML + WindowMode + SpectrumProcessing + Dynamic + XCorr + MzSpectrum + MsDataScan): 111/111 pass.

Backward compatibility

Strictly additive on the public API:

• MzSpectrum's existing 3-arg ctor still works (delegates to new 4-arg with chargeArray=null).
• MzSpectrum.ChargeArray is new (no existing caller could be using it).
• MsDataScan's chargeArray ctor parameter still works; the wrapping path keeps every existing call site compiling.
• MsDataScan.ChargeArray getter still works (now delegates).
• WindowModeHelper.Run's 5-parameter signature preserved.

The only behavior change for existing consumers: MzSpectrum.XCorrPrePreprocessing now drops ChargeArray (previously it would have left a length-mismatched stale array). For any caller that was not using ChargeArray this is a no-op. For one that was, the behavior shifts from "silent corruption" to "explicit null", which is what we want.

Other things we noticed but did not touch

While digging into this, two adjacent items caught our eye. Out of scope for this PR, listing here so they do not slip:

1. FilteringParams.MinimumAllowedIntensityRatioToBasePeakM - public property has a stray trailing M. The constructor parameter is minimumAllowedIntensityRatioToBasePeak (no M). Smells like an incomplete refactor and produces a mildly confusing developer experience (caused a CS1739 while we were writing the test for Keep MS:1000516 charge array aligned through reader reorder/trim points #1062). Easy property rename + obsolete-attribute on the old name if anyone agrees.

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.

Files changed

File	Notes
mzLib/MassSpectrometry/MzSpectrum.cs	New ChargeArray property, new 4-arg ctor with length validation, XCorr clears charges
mzLib/MassSpectrometry/MsDataScan.cs	Delegating ChargeArray, ctor wrap/conflict logic
mzLib/MassSpectrometry/WindowModeHelper.cs	Charge-aware Run overload
mzLib/Readers/MzML/Mzml.cs	Reader-side permutation sync (both paths) + new MzSpectrum ctor + SortInPlaceByKey helper
mzLib/Test/FileReadingTests/SpectraFileReading/TestMzML.cs	5 new architectural tests + 3 reader-side regression tests from #1062

[pcarvalho75]

Pre-empted the codecov patch-coverage flag (it fired on #1062 against the same code patterns). Pushed d4abb389 with four additional tests targeting the defensive branches the happy-path tests didn't reach:

• ChargeArrayThroughDynamicReaderPath - mirrors the zero-intensity-trim test through InitiateDynamicConnection + GetOneBasedScanFromDynamicConnection so both reader paths are exercised. Until now, all three reader-side regression tests went through LoadAllStaticData only.
• ChargeArrayDroppedWhenLengthMismatchedOnDisk - writes a normal scan, then surgically appends one extra 32-bit float to the chargeArray's base64-encoded <binary> element on disk so the decoded length is N+1 vs N peaks. Confirms the reader DROPS the misaligned chargeArray rather than shipping it (the defensive branch in both reader paths).
• WindowModeHelperLengthMismatchedChargeArrayDropped - direct unit test of the length-mismatch null-out branch in WindowModeHelper.Run, plus that mz/intensity processing still completes correctly without charges.
• WindowModeHelperEmptyWindowPreservesChargeAlignment - 2 windows over [0..1000], both peaks land in window 1; window 2 is empty. Asserts charges still pair correctly with mzs after the per-window selection's empty-window branch is hit on the charge-aware path.

15/15 charge tests pass; 115/115 in the broader regression sweep (TestMzML + WindowMode + SpectrumProcessing + Dynamic + XCorr + MzSpectrum + MsDataScan).

Nothing else outstanding from our side - over to you.

[nbollis]

Please take a look at the failing test. An issue seems to be persisting with indexing.

[codecov]

Codecov Report

❌ Patch coverage is 97.38220% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.73%. Comparing base (646e218) to head (5d444bd).
⚠️ Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
mzLib/Readers/MzML/Mzml.cs	96.20%	1 Missing and 2 partials ⚠️
mzLib/MassSpectrometry/MsDataScan.cs	90.47%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1063      +/-   ##
==========================================
+ Coverage   81.45%   81.73%   +0.28%     
==========================================
  Files         366      369       +3     
  Lines       46899    47609     +710     
  Branches     5556     5668     +112     
==========================================
+ Hits        38200    38912     +712     
+ Misses       7620     7600      -20     
- Partials     1079     1097      +18     

Files with missing lines	Coverage Δ
mzLib/MassSpectrometry/MzSpectra/MzSpectrum.cs	63.78% <100.00%> (+2.73%)	⬆️
mzLib/MassSpectrometry/WindowModeHelper.cs	99.48% <100.00%> (+0.25%)	⬆️
mzLib/MassSpectrometry/MsDataScan.cs	88.83% <90.47%> (+<0.01%)	⬆️
mzLib/Readers/MzML/Mzml.cs	88.67% <96.20%> (+2.64%)	⬆️

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:

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