casanovo/denovo/dataloaders.py (1)

427-441: 🎯 Functional Correctness | 🟠 Major

Handle unit-annotated retention times from pyteomics—convert minutes/hours to seconds.

The function contract specifies returning retention time in seconds, but float() on pyteomics unitfloat objects strips the unit_info without converting. For mzML/mzXML files where "scan start time" is stored in minutes (unit_info='minute'), this returns the raw numeric value instead of the required seconds. For example, 5.5 minutes becomes 5.5 instead of 330.0 seconds. MGF's "rtinseconds" is unaffected as it is already in seconds.

Suggested fix

 def _get_retention_time(spectrum: dict[str, Any]) -> float:
+    def _to_seconds(rt: Any) -> float:
+        value = float(rt)
+        unit = getattr(rt, "unit_info", None)
+        if unit == "minute":
+            return value * 60.0
+        if unit == "hour":
+            return value * 3600.0
+        return value
+
     # mzML / mzXML top-level keys
     for key in ("scan start time", "retention time", "retentionTime"):
         if key in spectrum:
-            return float(spectrum[key])
+            return _to_seconds(spectrum[key])
+
     # mzML nested scanList
     scans = spectrum.get("scanList", {}).get("scan") or []
     if scans and "scan start time" in scans[0]:
-        return float(scans[0]["scan start time"])
+        return _to_seconds(scans[0]["scan start time"])
+
     # MGF params block
     params = spectrum.get("params", {})
     for key in ("rtinseconds", "rtinsec"):
         if key in params:
-            return float(params[key])
+            return _to_seconds(params[key])

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@casanovo/denovo/dataloaders.py` around lines 427 - 441, The function
retrieves retention time values from various sources but uses float() which
strips unit information without converting to the required seconds unit. For the
mzML/mzXML top-level "scan start time" and "retention time" keys, and the nested
"scan start time" within the scanList, check if the retrieved value has a
unit_info attribute indicating the unit is 'minute', and if so multiply by 60 to
convert to seconds before returning. For the mzML nested scanList scan start
time check in scans[0], apply the same unit conversion logic. The MGF params
block keys (rtinseconds and rtinsec) are already in seconds, so those require no
conversion. Ensure all return statements in the function account for proper unit
conversion.

tests/unit_tests/test_unit.py (1)

3220-3245: 🎯 Functional Correctness | 🔵 Trivial

Add unit-aware RT cases to lock the "seconds" contract.

The test set currently validates key presence and shape, but not unit conversion behavior. Since pyteomics can attach unitfloat objects with unit_info attributes (e.g., for minute or hour durations), adding test cases with unit-annotated inputs would prevent regressions if unit normalization logic is implemented.

🧪 Suggested extension

 `@pytest.mark.parametrize`(
     "spectrum, expected",
     [
+        # Unit-annotated times should be normalized to seconds
+        (
+            {"scan start time": pyteomics.auxiliary.structures.unitfloat(2.0, "minute")},
+            120.0,
+        ),
+        (
+            {"scanList": {"scan": [{"scan start time": pyteomics.auxiliary.structures.unitfloat(0.5, "hour")}]}},
+            1800.0,
+        ),
     ],
 )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit_tests/test_unit.py` around lines 3220 - 3245, The
test_get_retention_time function parametrize decorator currently lacks test
cases with unit-annotated inputs that pyteomics can provide through unitfloat
objects with unit_info attributes. Add new parametrized test cases to the
decorator that include unitfloat objects with different time units (such as
minutes and hours) to verify that the _get_retention_time function correctly
normalizes all retention time values to seconds regardless of their input unit,
ensuring this unit conversion behavior is protected against future regressions.