Improve mzML reader parsing for filter strings and missing fields - #122
Open
falconsmilie wants to merge 8 commits into
Open
Improve mzML reader parsing for filter strings and missing fields#122falconsmilie wants to merge 8 commits into
falconsmilie wants to merge 8 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves mzML reader robustness in
alpharaw/mzml.py.The main fix is for NCE parsing from mzML scan filter strings. The previous implementation attempted to access
dict.getusing subscript syntax, which meant valid filter strings could silently fall back tonp.nanbecause the error was caught by a broad exception handler.This PR also removes duplicated charge-state parsing and adds safer handling for missing or empty mzML fields.
What changed
Fixed filter string NCE parsing
Previously, the filter string was accessed like this:
Since
getis a method, this raises aTypeError.The PR changes this to normal
.get("filter string")access and routes the result through a small helper:This keeps the behaviour close to the original intended logic while fixing the parsing bug.
Removed duplicated charge state parsing
Charge state is now parsed in one place.
Expected behaviour:
int00Improved missing-field handling
The parser now handles several optional mzML fields more safely, especially around precursor data.
This includes safer handling for:
Added empty import guard
_import()now avoids callingnp.concatenate(...)on empty lists.When no peak arrays are available, the reader returns empty arrays with the expected dtypes instead of failing with a NumPy error.
Tests added
This PR adds mzML reader tests covering:
Behaviour note
This PR does not intentionally change the public meaning of
precursor_mz.There is a related question around whether
precursor_mzshould useselected ion m/zor the isolation-window target m/z. I have kept that separate from this PR because it may affect downstream behaviour and seems worth confirming with maintainers first.Validation
Suggested local checks:
Checklist
@hcdfilter strings.@cidfilter strings.np.nan.np.nan.0.0.np.concatenate([]).To-do list (outside contributers only)