Skip to content

Read scan time unit from unitAccession, handle missing scan time - #414

Merged
MKoesters merged 1 commit into
pymzml:devfrom
aaronkanzer:fix/scan-time-unit-accession
Oct 27, 2026
Merged

MKoesters merged 1 commit into
pymzml:devfrom
aaronkanzer:fix/scan-time-unit-accession

Conversation

@aaronkanzer

Copy link
Copy Markdown
Contributor

Spectrum.scan_time_in_minutes() raises in three cases:

  1. The cvParam has unitAccession="UO:0000010" but no unitName: Exception: Time unit 'unicorns' unknown. In the mzML 1.1 schema unitName is optional, and unitAccession is what identifies the unit.
  2. The unit is millisecond. This should work, but the second branch is an if where it should be an elif, so the millisecond value falls through to the else and raises.
  3. The spectrum has no scan start time: AttributeError: 'NoneType' object has no attribute 'lower'. pyteomics/tests/tiny.pwiz.1.1.mzML (written by ProteoWizard) has such a spectrum (scan=21). Scan start time is not required by the mzML mapping rules.

Fix: fall back to the UO accession (UO:0000028, UO:0000010, UO:0000031, UO:0000032) when unitName is missing, turn the if into an elif, and return None when there is no scan start time. An unknown unit still raises, as before.

Tested:

  • New tests/scan_time_unit_test.py (5 tests). 3 fail on dev and all 5 pass with the fix.
  • Full suite: 135 passed before, 140 passed after (same 2 plotly-only failures, 3 skipped).
  • tiny.pwiz.1.1.mzML now reads to the end, with scan=21 returning None.

scan_time_in_minutes() raised 'Time unit unicorns unknown' when the
scan start time cvParam had a unitAccession but no unitName (unitName is
optional in the mzML schema), raised for millisecond because of an if
that should be elif, and raised AttributeError for spectra without a
scan start time. Fall back to the UO accession for the unit name, fix
the elif, and return None when there is no scan start time.
@sonarqubecloud

Copy link
Copy Markdown

@MKoesters

Copy link
Copy Markdown
Member

Thanks for the thorough write-up and tests, looks solid to me!
I'll check if there is a way around hardcoding the terms and using OboTranslator, but I think loading the uo.obo is anyway an overkill for this PR.
I might raise an issue for that though.

I'll approve and merge once I had a closer look later this week.

Best
Manuel

@aaronkanzer

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough write-up and tests, looks solid to me! I'll check if there is a way around hardcoding the terms and using OboTranslator, but I think loading the uo.obo is anyway an overkill for this PR. I might raise an issue for that though.

I'll approve and merge once I had a closer look later this week.

Best Manuel

Thanks Manuel! Agreed the OBO lookup would be a nicer long-term fix, happy to help there if useful.

@MKoesters
MKoesters self-requested a review October 26, 2026 13:31
@MKoesters
MKoesters merged commit 77cfb1b into pymzml:dev Oct 27, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants