diff --git a/alphabase/spectral_library/translate.py b/alphabase/spectral_library/translate.py index 57d693a0..3af25210 100644 --- a/alphabase/spectral_library/translate.py +++ b/alphabase/spectral_library/translate.py @@ -7,6 +7,7 @@ """ import multiprocessing as mp +import warnings import pandas as pd import tqdm @@ -44,8 +45,8 @@ "get_precursor_mz", # this module's own "WritingProcess", - "speclib_to_single_df", "speclib_to_swath_df", + "speclib_to_single_df", "translate_to_tsv", ] @@ -73,7 +74,7 @@ def _precursors_to_swath_df( # noqa: PLR0913 ) -> pd.DataFrame: """Convert precursor and fragment frames to a SWATH transition list. - The dataframe-in form of :func:`speclib_to_single_df`, so that one batch of + The dataframe-in form of :func:`speclib_to_swath_df`, so that one batch of precursors can be converted without standing up a `SpecLibBase` around it. """ df = pd.DataFrame() @@ -133,7 +134,7 @@ def _precursors_to_swath_df( # noqa: PLR0913 return df -def speclib_to_single_df( +def speclib_to_swath_df( speclib: SpecLibBase, *, translate_mod_dict: dict = None, @@ -145,10 +146,9 @@ def speclib_to_single_df( modloss: str = "H3PO4", verbose=True, ) -> pd.DataFrame: - """Convert an alphabase library into a SWATH/Spectronaut transition list. + """Convert an alphabase library to a SWATH/Spectronaut transition-list dataframe. - The in-memory counterpart of :func:`translate_to_tsv`, which writes the same - table to a file. + :func:`translate_to_tsv` writes the same table to a file. Parameters ---------- @@ -167,7 +167,7 @@ def speclib_to_single_df( Returns ------- pd.DataFrame - a single dataframe in the SWATH-like format + One row per precursor/fragment pair, in SWATH column names. """ return _precursors_to_swath_df( @@ -185,22 +185,15 @@ def speclib_to_single_df( ) -def speclib_to_swath_df( - speclib: SpecLibBase, - *, - keep_k_highest_fragments: int = 12, - min_frag_mz=200, - max_frag_mz=2000, - min_frag_intensity=0.01, -) -> pd.DataFrame: - speclib_to_single_df( - speclib, - translate_mod_dict=None, - keep_k_highest_fragments=keep_k_highest_fragments, - min_frag_mz=min_frag_mz, - max_frag_mz=max_frag_mz, - min_frag_intensity=min_frag_intensity, +def speclib_to_single_df(speclib: SpecLibBase, **kwargs) -> pd.DataFrame: + """Deprecated alias of :func:`speclib_to_swath_df`.""" + warnings.warn( + "speclib_to_single_df is deprecated; use speclib_to_swath_df, which names the " + "format it produces.", + FutureWarning, + stacklevel=2, ) + return speclib_to_swath_df(speclib, **kwargs) class WritingProcess(mp.Process): @@ -248,7 +241,7 @@ def translate_to_tsv( Fragment m/z range; fragments outside it are dropped. Pass 0 for no lower bound and `np.inf` for no upper bound. - See :func:`speclib_to_single_df`, whose parameters this shares, for the rest. + See :func:`speclib_to_swath_df`, whose parameters this shares, for the rest. """ if multiprocessing: queue_size = 1000000 // batch_size @@ -295,3 +288,11 @@ def translate_to_tsv( "Translation finished, it will take several minutes to export the rest precursors to the tsv file..." ) writing_process.join() + if writing_process.exitcode: + raise RuntimeError( + f"the process writing {tsv} exited with code " + f"{writing_process.exitcode}, so the file is incomplete; its traceback " + 'is above. A script with no `if __name__ == "__main__":` guard is the ' + "usual cause on macOS and Windows. Pass multiprocessing=False to write " + "from this process instead." + ) diff --git a/alphabase/spectral_library/translate_core.py b/alphabase/spectral_library/translate_core.py index 360c9369..f9ed6fca 100644 --- a/alphabase/spectral_library/translate_core.py +++ b/alphabase/spectral_library/translate_core.py @@ -19,13 +19,16 @@ from alphabase.numba_wrapper import numba_njit from alphabase.peptide.precursor import update_precursor_mz from alphabase.psm_reader.keys import ConstantsClass, PsmDfCols -from alphabase.utils import explode_multiple_columns -# Candidate precursor columns in order of precedence, for libraries that carry more than -# one. The `*_pred` names are peptdeep's prediction outputs, which take priority over a -# measured value; `irt_pred` outranks `rt_pred` because an indexed RT is what a -# third-party library wants. -RT_COLUMNS = ["irt_pred", "rt_pred", PsmDfCols.RT, "irt", PsmDfCols.RT_NORM] +# Candidate precursor columns, in order of precedence. +RT_COLUMNS = [ + "irt_pred", + "rt_pred", + "rt_norm_pred", + PsmDfCols.RT, + "irt", + PsmDfCols.RT_NORM, +] MOBILITY_COLUMNS = ["mobility_pred", PsmDfCols.MOBILITY] CCS_COLUMNS = ["ccs_pred", PsmDfCols.CCS] @@ -56,17 +59,6 @@ class FragmentTableCols(metaclass=ConstantsClass): LOSS_TYPE = "loss_type" -# the per-fragment columns, in the order the exports emit them -FRAGMENT_VALUE_COLUMNS = [ - FragmentTableCols.FRAG_TYPE, - FragmentTableCols.MZ, - FragmentTableCols.INTENSITY, - FragmentTableCols.CHARGE, - FragmentTableCols.SERIES_NUMBER, - FragmentTableCols.LOSS_TYPE, -] - - def get_precursor_mz(precursor_df: pd.DataFrame) -> pd.Series: """Return the precursors' m/z, leaving `precursor_df` alone. @@ -186,7 +178,8 @@ def _get_frag_info_from_column_name(column: str) -> tuple: """Split a fragment column name into `(frag_type, loss_type, charge)`. For example `y_modloss_z2` -> `('y', 'modloss', '2')` and `b_z1` -> `('b', - 'noloss', '1')`. The charge is left as a string, as it is only written out. + 'noloss', '1')`. The charge stays a string because numba cannot parse one; + :func:`fragment_table` casts the collected column. """ idx = column.rfind("_") frag_type = column[:idx] @@ -264,7 +257,9 @@ def get_fragment_table( # noqa: PLR0913 Returns ------- pd.DataFrame - One row per kept fragment, in :class:`FragmentTableCols` columns. + One row per kept fragment, in :class:`FragmentTableCols` columns. `mz` and + `intensity` keep the dtype of the frame they came from; the charge and series + number are integers. """ frag_columns = fragment_mz_df.columns.to_numpy().astype("U") @@ -280,12 +275,12 @@ def get_fragment_table( # noqa: PLR0913 ) max_frag_mz = np.inf - frag_types = [] - frag_losses = [] - frag_charges = [] - frag_masses = [] - frag_intensities = [] - frag_numbers = [] + frag_types: list = [] + frag_losses: list = [] + frag_charges: list = [] + frag_numbers: list = [] + frag_masses: list = [] + frag_intensities: list = [] frag_idx_ranges = zip(frag_start_idx, frag_stop_idx) if verbose: frag_idx_ranges = tqdm.tqdm(frag_idx_ranges) @@ -319,27 +314,27 @@ def get_fragment_table( # noqa: PLR0913 infos = [_get_frag_info_from_column_name(column) for column in columns] types, losses, charges = zip(*infos) if infos else ((), (), ()) - frag_types.append(types) - frag_losses.append(losses) - frag_charges.append(charges) + frag_types.extend(types) + frag_losses.extend(losses) + frag_charges.extend(charges) + frag_numbers.extend(_get_frag_num(columns, rows, frag_len)) frag_masses.append(masses[idx_in_df]) frag_intensities.append(intens[idx_in_df]) - frag_numbers.append(_get_frag_num(columns, rows, frag_len)) - fragments_df = pd.DataFrame( + return pd.DataFrame( { - FragmentTableCols.PRECURSOR_ROW: np.arange(len(frag_start_idx)), + FragmentTableCols.PRECURSOR_ROW: np.repeat( + np.arange(len(frag_start_idx)), + [len(kept) for kept in frag_masses], + ), FragmentTableCols.FRAG_TYPE: frag_types, - FragmentTableCols.MZ: frag_masses, - FragmentTableCols.INTENSITY: frag_intensities, - FragmentTableCols.CHARGE: frag_charges, - FragmentTableCols.SERIES_NUMBER: frag_numbers, + FragmentTableCols.MZ: np.concatenate(frag_masses), + FragmentTableCols.INTENSITY: np.concatenate(frag_intensities), + FragmentTableCols.CHARGE: np.array(frag_charges, dtype=np.int8), + FragmentTableCols.SERIES_NUMBER: np.array(frag_numbers, dtype=np.int64), FragmentTableCols.LOSS_TYPE: frag_losses, } ) - fragments_df = explode_multiple_columns(fragments_df, FRAGMENT_VALUE_COLUMNS) - # a precursor that kept nothing explodes to one all-NaN row; drop those - return fragments_df.dropna(subset=[FragmentTableCols.MZ]) def join_fragments( diff --git a/alphabase/spectral_library/translate_diann.py b/alphabase/spectral_library/translate_diann.py index c2b5d34c..bdaf5ffb 100644 --- a/alphabase/spectral_library/translate_diann.py +++ b/alphabase/spectral_library/translate_diann.py @@ -62,8 +62,10 @@ class DiannParquetCols(metaclass=ConstantsClass): SOURCE_ID = "Source.Id" -# the DIA-NN names for the canonical fragment columns, in output order +# the DIA-NN names for the canonical fragment columns, in output order. `precursor_row` +# keeps its name for the base-peak flag; the schema selection at the end drops it. DIANN_FRAGMENT_COLUMNS = { + FragmentTableCols.PRECURSOR_ROW: FragmentTableCols.PRECURSOR_ROW, FragmentTableCols.FRAG_TYPE: DiannParquetCols.FRAGMENT_TYPE, FragmentTableCols.MZ: DiannParquetCols.PRODUCT_MZ, FragmentTableCols.INTENSITY: DiannParquetCols.RELATIVE_INTENSITY, @@ -227,10 +229,10 @@ def _precursors_to_diann_df( # noqa: PLR0913 ] = modloss df = df.reset_index(drop=True) - # Flags: base bit on all fragments, base-peak bit on each precursor's most intense one df[DiannParquetCols.FLAGS] = _DIANN_FLAG_BASE if len(df): - base_peak_idx = df.groupby(DiannParquetCols.PRECURSOR_ID, sort=False)[ + # by row: `Precursor.Id` repeats when a library holds the same precursor twice + base_peak_idx = df.groupby(FragmentTableCols.PRECURSOR_ROW, sort=False)[ DiannParquetCols.RELATIVE_INTENSITY ].idxmax() df.loc[base_peak_idx, DiannParquetCols.FLAGS] |= _DIANN_FLAG_FIRST_FRAGMENT diff --git a/nbs_tests/spectral_library/translate.ipynb b/nbs_tests/spectral_library/translate.ipynb index 99414857..47c91b7e 100644 --- a/nbs_tests/spectral_library/translate.ipynb +++ b/nbs_tests/spectral_library/translate.ipynb @@ -33,7 +33,7 @@ "import pandas as pd\n", "\n", "from alphabase.spectral_library.base import SpecLibBase\n", - "from alphabase.spectral_library.translate import create_modified_sequence, speclib_to_single_df, translate_to_tsv" + "from alphabase.spectral_library.translate import create_modified_sequence, speclib_to_swath_df, translate_to_tsv" ] }, { @@ -538,10 +538,10 @@ "spec_lib._precursor_df = precursor_df\n", "spec_lib._fragment_intensity_df = frag_mass_df.copy()\n", "spec_lib._fragment_mz_df = frag_mass_df.copy()\n", - "df = speclib_to_single_df(spec_lib, min_frag_mz=300, max_frag_mz=1800)\n", + "df = speclib_to_swath_df(spec_lib, min_frag_mz=300, max_frag_mz=1800)\n", "assert (df.FragmentMz>=300).all()\n", "assert (df.FragmentMz<=1800).all()\n", - "df = speclib_to_single_df(spec_lib, min_frag_mz=200, min_frag_nAA=3)\n", + "df = speclib_to_swath_df(spec_lib, min_frag_mz=200, min_frag_nAA=3)\n", "assert (df.FragmentNumber>=3).all()" ] }, @@ -846,7 +846,7 @@ "spec_lib._precursor_df = precursor_df\n", "spec_lib._fragment_intensity_df = frag_mass_df.copy()\n", "spec_lib._fragment_mz_df = frag_mass_df.copy()\n", - "speclib_sdf = speclib_to_single_df(spec_lib)\n", + "speclib_sdf = speclib_to_swath_df(spec_lib)\n", "with tempfile.TemporaryFile('w+') as f:\n", " translate_to_tsv(spec_lib, f, batch_size=2, multiprocessing=False)\n", " f.seek(0)\n", diff --git a/tests/unit/spectral_library/test_translate.py b/tests/unit/spectral_library/test_translate.py index 3ac089ce..4f0ad135 100644 --- a/tests/unit/spectral_library/test_translate.py +++ b/tests/unit/spectral_library/test_translate.py @@ -2,15 +2,6 @@ These tests pin the behaviour of `alphabase.spectral_library.translate` as it is today, so that the upcoming restructuring can be shown to change nothing. - -Two tests still pin *buggy* behaviour that a later commit fixes. Each carries a -`CHARACTERIZATION (bug)` note in its docstring: - -1. `rt_norm_pred` is not accepted as a retention time column -2. the exploded fragment columns are object dtype, with a string `FragmentCharge` - -One more, `test_speclib_to_swath_df_returns_none`, pins a function that a later -commit removes rather than fixes. """ import hashlib @@ -20,6 +11,7 @@ import pytest from alphabase.peptide.fragment import get_charged_frag_types +from alphabase.spectral_library import translate from alphabase.spectral_library.base import SpecLibBase from alphabase.spectral_library.reader import LibraryReaderBase from alphabase.spectral_library.translate import ( @@ -100,7 +92,7 @@ def _build_speclib(**extra_columns) -> SpecLibBase: def _export(speclib: SpecLibBase, **kwargs) -> pd.DataFrame: """Run the in-memory export with the progress bar off.""" - return speclib_to_single_df(speclib, verbose=False, **kwargs) + return speclib_to_swath_df(speclib, verbose=False, **kwargs) def _unfiltered(speclib: SpecLibBase, **kwargs) -> pd.DataFrame: @@ -169,15 +161,16 @@ def test_modified_sequence_uses_translate_mod_dict() -> None: @pytest.mark.parametrize( ("present", "expected"), [ - (["irt_pred", "rt_pred", "rt", "irt", "rt_norm"], "irt_pred"), - (["rt_pred", "rt", "irt", "rt_norm"], "rt_pred"), + (["irt_pred", "rt_pred", "rt_norm_pred", "rt", "irt", "rt_norm"], "irt_pred"), + (["rt_pred", "rt_norm_pred", "rt", "irt", "rt_norm"], "rt_pred"), + (["rt_norm_pred", "rt", "irt", "rt_norm"], "rt_norm_pred"), (["rt", "irt", "rt_norm"], "rt"), (["irt", "rt_norm"], "irt"), (["rt_norm"], "rt_norm"), ], ) def test_rt_column_precedence(present: list, expected: str) -> None: - """RT is taken from the first present of irt_pred, rt_pred, rt, irt, rt_norm.""" + """RT comes from the first present candidate, predictions before measurements.""" speclib = _build_speclib() # give each candidate column a distinct value, so `RT` identifies its source values = {name: float(i + 1) for i, name in enumerate(present)} @@ -188,17 +181,25 @@ def test_rt_column_precedence(present: list, expected: str) -> None: assert _export(speclib)["RT"].unique().tolist() == [values[expected]] -def test_rt_norm_pred_is_not_accepted() -> None: - """CHARACTERIZATION (bug): `rt_norm_pred` is not a recognised RT column. +def test_rt_norm_pred_is_accepted() -> None: + """A library carrying only `rt_norm_pred` exports, rather than being rejected. - peptdeep writes it alongside `rt_pred`, so a library carrying only - `rt_norm_pred` is rejected. A later commit adds it to the candidates. + peptdeep writes `rt_norm_pred` alongside `rt_pred`, so this matters for a + library whose `rt_pred` was dropped. """ speclib = _build_speclib() speclib._precursor_df = speclib._precursor_df.rename( columns={"rt_pred": "rt_norm_pred"} ) + assert _export(speclib)["RT"].notna().all() + + +def test_export_without_any_rt_column_is_rejected() -> None: + """A library with no retention time at all is still an error.""" + speclib = _build_speclib() + speclib._precursor_df = speclib._precursor_df.drop(columns=["rt_pred"]) + with pytest.raises(ValueError, match="must contain the RT columns"): _export(speclib) @@ -370,42 +371,34 @@ def test_disabled_mz_window_skips_empty_fragment_slots() -> None: assert with_loss <= {"SVIVSPYSTGAK", "LHDSTPPPYK"} -def test_exploded_fragment_columns_are_object_dtype() -> None: - """CHARACTERIZATION (bug): the fragment columns are objects, not numbers. +def test_fragment_columns_are_typed() -> None: + """The fragment columns are numbers, and m/z and intensity keep their own dtype. - Unlike the DIA-NN export, this one does no dtype casting, so `FragmentMz` and - friends are object-dtype, and `FragmentCharge` holds strings -- it is parsed - out of the fragment column name and never converted. That costs ~14x the - memory of the equivalent typed columns, and makes arithmetic on the charge - concatenate rather than add. The cells already hold correctly typed numpy - scalars, so a later commit types the columns from their source dtype, leaving - the written tsv byte-identical. + The fixture builds a float32 m/z frame and a float64 intensity frame, so the + two carry through independently rather than being cast to one width. """ - df = _export(_build_speclib()) + speclib = _build_speclib() + df = _export(speclib) - for column in ( - "FragmentType", - "FragmentMz", - "RelativeIntensity", - "FragmentCharge", - "FragmentNumber", - "FragmentLossType", - ): - assert df[column].dtype == object, column + assert df["FragmentMz"].dtype == speclib.fragment_mz_df.to_numpy().dtype + assert ( + df["RelativeIntensity"].dtype == speclib.fragment_intensity_df.to_numpy().dtype + ) + assert df["FragmentCharge"].dtype.kind == "i" + assert df["FragmentNumber"].dtype.kind == "i" + # the two label columns stay strings + assert df["FragmentType"].dtype == object + assert df["FragmentLossType"].dtype == object - assert set(map(type, df["FragmentCharge"])) == {str} - assert set(df["FragmentCharge"]) == {"1", "2"} + assert set(df["FragmentCharge"]) == {1, 2} -def test_speclib_to_swath_df_returns_none() -> None: - """CHARACTERIZATION: the wrapper discards its result, returning None. +def test_speclib_to_single_df_is_a_deprecated_alias() -> None: + """The old name still works, warns, and returns what the new one returns.""" + with pytest.warns(FutureWarning, match="use speclib_to_swath_df"): + aliased = speclib_to_single_df(_build_speclib(), verbose=False) - It is a parameter subset of `speclib_to_single_df` that has been missing its - `return` since 93aa2fa, so it is only ever called for the side effect of - editing the library. Pinned as the evidence that removing it later is safe: - nothing can depend on a function that has only ever returned None. - """ - assert speclib_to_swath_df(_build_speclib()) is None + pd.testing.assert_frame_equal(aliased, _export(_build_speclib())) def test_translate_to_tsv_matches_the_in_memory_export(tmp_path) -> None: @@ -421,8 +414,6 @@ def test_translate_to_tsv_matches_the_in_memory_export(tmp_path) -> None: assert len(lines) == len(expected) + 1 written = pd.read_csv(tsv, sep="\t") assert list(written.columns) == list(expected.columns) - numeric = ["FragmentMz", "RelativeIntensity", "FragmentCharge", "FragmentNumber"] - expected[numeric] = expected[numeric].apply(pd.to_numeric) pd.testing.assert_frame_equal( written.reset_index(drop=True), expected.reset_index(drop=True), @@ -464,7 +455,7 @@ def test_translate_to_tsv_disabled_mz_window_matches_the_in_memory_export( `translate_to_tsv` used to mask by m/z without checking whether the window was disabled, so 0/0 -- the documented way to turn the filter off -- zeroed every real fragment and wrote a file of nothing but empty slots, while - `speclib_to_single_df` given the same arguments kept the real ones. + `speclib_to_swath_df` given the same arguments kept the real ones. """ tsv = str(tmp_path / "lib.tsv") translate_to_tsv( @@ -481,8 +472,6 @@ def test_translate_to_tsv_disabled_mz_window_matches_the_in_memory_export( assert (written["FragmentMz"] > 0).all() expected = _unfiltered(_build_speclib()) - numeric = ["FragmentMz", "RelativeIntensity", "FragmentCharge", "FragmentNumber"] - expected[numeric] = expected[numeric].apply(pd.to_numeric) pd.testing.assert_frame_equal( written.reset_index(drop=True), expected.reset_index(drop=True), @@ -513,3 +502,35 @@ def keys(df: pd.DataFrame) -> set: return set(zip(df["sequence"], df["mod_sites"], df["charge"].astype(int))) assert keys(reader.precursor_df) == keys(speclib.precursor_df) + + +class _DeadWriter: + """Stands in for a `WritingProcess` whose child died without writing.""" + + exitcode = 1 + + def __init__(self, task_queue, tsv) -> None: + pass + + def start(self) -> None: + pass + + def join(self) -> None: + pass + + +def test_translate_to_tsv_raises_when_the_writing_process_dies( + tmp_path, monkeypatch +) -> None: + """A writer that died is an error, not a quietly truncated file. + + `multiprocessing=True` is the default, and the writer process dies before it + writes anything if the calling script has no `if __name__ == "__main__":` + guard -- the norm on the spawn platforms, macOS and Windows. Nothing checked + on it, so the export printed its success message and returned normally, + leaving a 0-byte tsv behind. + """ + monkeypatch.setattr(translate, "WritingProcess", _DeadWriter) + + with pytest.raises(RuntimeError, match="exited with code 1"): + translate_to_tsv(_build_speclib(), str(tmp_path / "lib.tsv")) diff --git a/tests/unit/spectral_library/test_translate_diann.py b/tests/unit/spectral_library/test_translate_diann.py index 18b9e35c..62410265 100644 --- a/tests/unit/spectral_library/test_translate_diann.py +++ b/tests/unit/spectral_library/test_translate_diann.py @@ -150,15 +150,16 @@ def _keys(df: pd.DataFrame) -> set: @pytest.mark.parametrize( ("present", "expected"), [ - (["irt_pred", "rt_pred", "rt", "irt", "rt_norm"], "irt_pred"), - (["rt_pred", "rt", "irt", "rt_norm"], "rt_pred"), + (["irt_pred", "rt_pred", "rt_norm_pred", "rt", "irt", "rt_norm"], "irt_pred"), + (["rt_pred", "rt_norm_pred", "rt", "irt", "rt_norm"], "rt_pred"), + (["rt_norm_pred", "rt", "irt", "rt_norm"], "rt_norm_pred"), (["rt", "irt", "rt_norm"], "rt"), (["irt", "rt_norm"], "irt"), (["rt_norm"], "rt_norm"), ], ) def test_speclib_to_diann_df_rt_column_precedence(present: list, expected: str) -> None: - """RT is taken from the first present of irt_pred, rt_pred, rt, irt, rt_norm.""" + """RT comes from the first present candidate, predictions before measurements.""" speclib = _build_speclib() # give each candidate column a distinct value, so `RT` identifies its source values = {name: float(i + 1) for i, name in enumerate(present)} @@ -174,15 +175,30 @@ def test_speclib_to_diann_df_rt_column_precedence(present: list, expected: str) assert df["RT"].unique().tolist() == [values[expected]] -def test_speclib_to_diann_df_rejects_rt_norm_pred() -> None: - """CHARACTERIZATION (bug): `rt_norm_pred` is not a recognised RT column. +def test_speclib_to_diann_df_accepts_rt_norm_pred() -> None: + """A library carrying only `rt_norm_pred` exports, rather than being rejected. - peptdeep writes it alongside `rt_pred`, so a library carrying only - `rt_norm_pred` is rejected. A later commit adds it to the candidates. + peptdeep writes `rt_norm_pred` alongside `rt_pred`, so this matters for a + library whose `rt_pred` was dropped. """ speclib = _build_speclib() speclib._precursor_df = speclib._precursor_df.rename(columns={"rt": "rt_norm_pred"}) + df = speclib_to_diann_df( + speclib, + min_frag_mz=0, + max_frag_mz=np.inf, + min_frag_intensity=0.0, + verbose=False, + ) + assert df["RT"].notna().all() + + +def test_speclib_to_diann_df_without_any_rt_column_is_rejected() -> None: + """A library with no retention time at all is still an error.""" + speclib = _build_speclib() + speclib._precursor_df = speclib._precursor_df.drop(columns=["rt"]) + with pytest.raises(ValueError, match="must contain a retention time column"): speclib_to_diann_df( speclib, @@ -193,13 +209,12 @@ def test_speclib_to_diann_df_rejects_rt_norm_pred() -> None: ) -def test_speclib_to_diann_df_flags_group_by_precursor_id() -> None: - """CHARACTERIZATION (bug): duplicate precursors share one base-peak flag. +def test_speclib_to_diann_df_flags_one_base_peak_per_precursor_row() -> None: + """Duplicate precursors each get their own base-peak flag. - `Flags` marks the base peak by grouping on `Precursor.Id`, so a library - holding the same precursor twice -- which `SpecLibBase.append` produces -- - gets one flag for the pair instead of one per precursor row. A later commit - groups by precursor row instead. + `Flags` marks the base peak per precursor *row*, so a library holding the same + precursor twice -- which `SpecLibBase.append` produces -- gets one flag each + rather than one for the pair. `Precursor.Id` cannot distinguish them. """ speclib = _build_speclib() speclib.append(_build_speclib()) @@ -213,9 +228,14 @@ def test_speclib_to_diann_df_flags_group_by_precursor_id() -> None: verbose=False, ) - # every precursor row is exported, but only the distinct ids get a base peak + # the duplicated precursors share an id, so the id count is half the row count assert df["Precursor.Id"].nunique() == n_precursors // 2 - assert int((df["Flags"] & (1 << 4) > 0).sum()) == n_precursors // 2 + assert int((df["Flags"] & (1 << 4) > 0).sum()) == n_precursors + + # and each flagged fragment is the most intense of its own precursor's block + flagged = df["Flags"] & (1 << 4) > 0 + for _, group in df.groupby("Precursor.Id", sort=False): + assert group[flagged.loc[group.index]].shape[0] == 2 def test_speclib_to_diann_df_leaves_the_source_library_untouched() -> None: