Adding remaining Koina models#1073
Conversation
…e, unrestricted CE - Add ParsedFragmentAnnotation record with neutral loss parsing - Extract API outputs by name instead of positional index - Fix intensity variable bug (was using m/z instead of intensities) - Support neutral loss mass calculation in fragment ion matching - Add CreateUnimodConverterAcceptAll for models accepting any modification - Make KoinaModelBase members protected for subclass extensibility - Remove collision energy restrictions on Prosit2020IntensityHCD - Move DetectabilityModel to AbstractClasses namespace
…veness, fix validation - Add BuildBatchedRequest helper + InputField record to KoinaModelBase, eliminating ~770 lines of duplicated dictionary construction across 37 models - Remove Guid.NewGuid() from batch IDs (simpler counter-based IDs); server ignores the id field for deduplication - Add 'default: throw' to all ParameterHandlingMode switches for future-proofing against new enum values - Replace positional Outputs[0] lookup in CollisionalCrossSectionModel with name-based "ccs" output resolution - Move crosslink beta-null check from AsyncThrottledPredictor loop into ValidateModelSpecificInputs for consistent validation - Remove stale Outputs.Count != 3 guards (ExtractOutputs validates by name) - Add virtual NumberOfPredictedFragmentIons to FragmentIntensityModel base; override in 19 concrete models for polymorphic access
…, thread-safety docs, RT ToArray
- Fix BaseFragmentIdentifier computed property: use IndexOf('-') instead of
string-length arithmetic that broke for multi-character neutral loss formulas
(e.g. H3PO4 would include the trailing dash in the base identifier)
- Add missing null-forgiving operator on ValidatedFullSequence in
PFly2024FineTuned.ToBatchedRequests (consistent with all other models)
- Document thread-safety caveat on FragmentIntensityModel,
CollisionalCrossSectionModel, CrosslinkFragmentIntensityModel,
and DetectabilityModel (previously only on RetentionTimeModel)
- Remove unnecessary .ToArray() in RetentionTimeModel.ResponseToPredictions
(List<string> already implements IReadOnlyList<string>)
…rt charge guard
- Remove Contains('+') pre-filter in GenerateLibrarySpectraFromPredictions
that silently dropped Altimeter (^ delimiter) and UniSpec annotations.
Parse failures are now caught by try/catch around ParseFragmentAnnotation
in the second loop, matching the ResponseToPredictions pattern.
- Replace tpLookup indexer (KeyNotFoundException risk) with TryGetValue
+ continue, consistent with ResponseToPredictions at line 458.
- Add ParameterWarning fallback in CollisionalCrossSectionModel realignment
(was SequenceWarning-only, unlike the other two model types).
- Change BaseFragmentIdentifier LastIndexOf('-') to IndexOf('-') to match
the dash-finding logic in ParseFragmentAnnotation.
- Revert the charge <= 0 guard in CollisionalCrossSectionModel.ValidateModelSpecificInputs
and add a TODO to revisit what empty AllowedPrecursorCharges means.
Phase 1 — Validation logic change:
- Change AllowedCollisionEnergies, AllowedInstrumentTypes, AllowedFragmentationTypes
base property types from HashSet<T> to HashSet<T>? with default null
- null = parameter not applicable to this model (skip validation entirely)
- empty HashSet = parameter IS required but any value accepted
- populated HashSet = parameter IS required AND value must be in set
- Split the combined 'missing required params' check into per-parameter checks
with individual error messages
Phase 2 — Model migration:
- Models that send collision_energies to Koina keep empty HashSet (required, any value):
All Prosit fragment models (except CID), UniSpec, AlphaPeptDeep, XLCMS2, XLNMS2
- Models that don't send collision_energies change to null (not applicable):
Prosit2020IntensityCID, all 7 Ms2Pip models, Prosit2023IntensityXLCMS3
- Altimeter keeps its populated range (already correct)
- XLCMS2 and XLNMS2 add explicit AllowedCollisionEnergies = {} override
(previously inherited now-changed base default)
- Add XML doc comments to AllowedPrecursorCharges, AllowedCollisionEnergies, AllowedInstrumentTypes, and AllowedFragmentationTypes in FragmentIntensityModel documenting the three-tier convention - Add same docs to AllowedPrecursorCharges and AllowedCollisionEnergies in CrosslinkFragmentIntensityModel - Update AllowedUnimodIds doc in KoinaModelBase to clarify it belongs to the converter layer (not parameter validation) with empty = accept none - Update TODO in CollisionalCrossSectionModel to reference the new null/empty convention and note AllowedPrecursorCharges isn't yet migrated
trishorts
left a comment
There was a problem hiding this comment.
Automated code review - 3 finding(s) at severity >= Major.
- Fix MapToInputFullSequence desynchronization bug in FragmentIntensityModel.cs - Correct Prosit2024IntensityXLNMS2 fragment ion count from 174 to 348 - Fix Prosit2020IntensityTMT TryCleanSequence return value
…nd batch type mismatches - FragmentIntensityModel: require non-empty constraint sets before rejecting null params - CrosslinkModelTests: handle null AllowedCollisionEnergies for CMS3 - Ms2PipImmunoHCD: correct MaxPeptideLength from 15 to 30 - CollisionalCrossSectionModel: add non-physical charge guard - CrosslinkFragmentIntensityModel: fix sequence validation warning propagation - CollisionEnergy: cast to float across all model impls (int? -> float) - UniSpec: case-insensitive instrument type matching - Ms2PipCIDTMT: N-terminal TMT/iTRAQ label validation - Crosslink benchmark: generate independent beta peptides - Prosit2024IntensityXLNMS2: correct fragment ion count to 348
…mzlib into KoinaRemainingModels
…raints to empty sets The TestFragmentIntensityModel constructor defaulted unspecified constraints (AllowedCollisionEnergies, AllowedInstrumentTypes, AllowedFragmentationTypes) to empty HashSets. Per the validation logic in FragmentIntensityModel.cs: - null = skip validation (not applicable) - empty set = parameter IS required but any value accepted This caused 4 tests to fail because empty sets made untested constraints required, while the test inputs provided null for those fields. Fix: default to null instead of empty sets, so only explicitly-passed constraints are validated.
…ient, add coverage
Audit follow-ups for the Koina prediction client:
- FragmentIntensityModel: route the MapToInputFullSequence branch through the
overridable ParseFragmentAnnotation and compute neutral-loss-aware m/z, matching
GenerateLibrarySpectraFromPredictions. Fixes a crash on non-'+' annotations
(Altimeter '^', UniSpec internal ions) and wrong m/z for neutral-loss ions.
- HTTP: back with a single process-wide HttpClient; per-request CancellationToken
timeout replaces the per-session client to avoid socket exhaustion under repeated
Predict() calls. Updated the five AsyncThrottledPredictor call sites.
- CollisionalCrossSectionModel: make AllowedPrecursorCharges nullable to follow the
null/empty/populated constraint convention; the charge >= 1 physical guard still
applies. Removes the standing TODO.
- DetectabilityModel: map ValidatedFullSequence from requestInputs rather than the
full ModelInputs list, so the reported sequence is correct when inputs are filtered.
- Prosit_2025_intensity_lac: send instrument types uppercased and accept them
case-insensitively. Koina's Prosit_Preprocess_instrument_types matches uppercase
and silently defaults unknown casing to LUMOS, so lowercase astral/eclipse were
silently mispredicted.
Tests (no network; run on every PR build):
- New unit fixtures for crosslink, CCS, detectability, fragment-intensity, and HTTP
covering TryCleanSequence, ValidateModelSpecificInputs, ResponseToPredictions,
the all-invalid realignment path, GenerateLibrarySpectraFromPredictions, and the
MapToInputFullSequence/neutral-loss parsing.
- Moved crosslink client-side validation tests out of the [Category("Koina")] fixture
so they count toward coverage.
PredictionClients line coverage ~49% -> ~66%; HTTP 0 -> 100%,
CrosslinkFragmentIntensityModel ~5% -> ~89%.
nbollis
left a comment
There was a problem hiding this comment.
Review Synthesis
Major (2)
mzLib/PredictionClients/Koina/AbstractClasses/CrosslinkFragmentIntensityModel.cs:88Crosslink mapping mode is configurable but never applied — the new crosslink base class adds aFragmentIonMappingModecontract, but the implementation never branches on it, so callers can requestMapToInputFullSequencewithout getting distinct behavior.- Summary-only:
mzLib/PredictionClients/Koina/AbstractClasses/CrosslinkFragmentIntensityModel.cs:383Crosslink response parsing still accepts mismatched output arrays — the new parser checks only annotation divisibility before indexingannotations,mz, andintensitiesin lockstep; it does not validate equal array lengths or enforce the fixed-sizeNumberOfPredictedFragmentIonscontract.
Minor (2)
mzLib/Test/KoinaTests/KoinaRequestBuildingTests.cs:91Crosslink request-building smoke test bypasses the new model-specific UNIMOD validation — the test seedsValidated*Sequencedirectly withUNIMOD:1896, so it never exercises XLNMS2's actual{4, 35, 1898}validation path.mzLib/Test/KoinaTests/KoinaRequestBuildingTests.cs:112Only the negative TMT label path is protected offline — the new offline TMT test covers rejection only, not the successful labeled/request-building path.
Additional summary-only concern
mzLib/PredictionClients/Koina/AbstractClasses/FragmentIntensityModel.cs:737Generated library spectra keep the original sequence after masses were rebuilt from the validated sequence — underMapToValidatedFullSequence, precursor and fragment masses are rebuilt fromValidatedFullSequence, butLibrarySpectrum.Sequence/Namestill useFullSequence, which can desynchronize the library identity from the actual spectral content.
Grouping: 5 approved concerns total → 3 inline findings submitted | 2 summary-only findings
- Remove unused FragmentIonMappingMode from the crosslink model hierarchy - Guard equal annotation/mz/intensity output array lengths in ResponseToPredictions - Label library spectra with the sequence the masses were built from - Add offline tests for XLNMS2 and TMT validation, TMT fragmentation type, and library labeling
# Conflicts: # mzLib/PredictionClients/Koina/AbstractClasses/FragmentIntensityModel.cs # mzLib/Test/KoinaTests/FragmentIntensityPrediction/Prosit2020IntensityHCDTests.cs
trishorts
left a comment
There was a problem hiding this comment.
Automated code review - 3 finding(s) at severity >= Major, posted as inline comments.
…name discovery test - Wrap CCS/Crosslink/RetentionTime/Detectability Predict in lock + Task.Run to avoid the documented single-threaded SynchronizationContext deadlock - Require a CancellationToken on HTTP requests and add a coarse backstop client timeout - Add deadlock regression tests and a live ModelName-resolves-to-Koina-endpoint test
trishorts
left a comment
There was a problem hiding this comment.
Automated code review - 5 finding(s) at severity >= Major, posted as inline comments.
|
@pcruzparri Thanks for the work on the 06-30 commits — the neutral-loss base-product fix ( I did want to check on two of the Major threads that are now marked resolved but whose files don't appear in the latest commits (
No rush — if these are genuinely no-ops or already-correct, a one-line note on each thread explaining why would let me close them out with confidence. Thanks! |
TLDR
Add 21 Koina prediction models, refactor model layer, introduce null/empty parameter convention
Summary
Adds 21 new concrete Koina model implementations across fragment intensity, CCS, crosslink, detectability, and retention time categories. Refactors the shared model infrastructure: deduplicates request batching, adds neutral loss parsing, switches from positional to name-based output extraction, and introduces a three-tier null/empty/populated convention for parameter validation (collision energy, instrument type, fragmentation type). The spline models are not included in this PR due to 1) lack of necessity and 2) different output convention.
Models Added
Fragment Intensity (16)
Prosit_2019_intensityProsit_2020_intensity_HCDProsit_2020_intensity_CIDProsit_2020_intensity_TMTHCD,CIDProsit_2023_intensity_timsTOFProsit_2024_intensity_citHCD,CIDProsit_2024_intensity_PTMs_glHCD,CID; 34 UNIMODsProsit_2025_intensity_lacHCD/CID; Inst:eclipse/astral/lumosProsit_2025_intensity_22PTMProsit_2025_intensity_40PTMProsit_2025_intensity_ptm2HCD,CIDAltimeter_2024_intensities^charge delimiterUniSpecQE/QEHFX/LUMOS/ELITE/VELOS/NONEAlphaPeptDeep_ms2_genericQE/LUMOS/TIMSTOF/SCIEXTOFms2pip_*CCS (2)
AlphaPeptDeep_ccs_genericIM2DeepCrosslink (3)
Prosit_2023_intensity_XL_CMS2Prosit_2024_intensity_XL_NMS2Prosit_2023_intensity_XL_CMS3Detectability (1)
pfly_2024_fine_tunedRetention Time (10, 9 new + existing Prosit2019iRT updated)
Prosit_2019_irtProsit_2020_irt_TMTProsit_2024_irt_citProsit_2024_irt_PTMs_glProsit_2025_irt_lacProsit_2025_irt_22_PTMProsit_2025_irt_40_PTMAlphaPeptDeep_rt_genericDeeplc_hela_hfChronologer_RTBreaking Changes
Null/empty convention for Allowed* parameter collections
AllowedCollisionEnergies,AllowedInstrumentTypes, andAllowedFragmentationTypeschanged fromHashSet<T>toHashSet<T>?. Previously an empty set meant "bypass validation entirely." Now:null{}(empty){a, b, c}null(e.g., Ms2Pip models for CE). Models that do send to Koina use{}(Prosit2019-2025 intensity models, UniSpec, AlphaPeptDeep) or a populated set (Altimeter with range 20–40, Prosit TMT/Cit/Lac/PTM with specific fragmentation types).Existing models updated
PFly2024FineTuned: namespace fix; null-forgiving operator inToBatchedRequestsProsit2019iRT,Prosit2020iRTTMT: updated to use new base infrastructureRetentionTimeModel: removed unnecessary.ToArray()callInfrastructure Changes
KoinaModelBase: newBuildBatchedRequesthelper andInputFieldrecord eliminates duplicated request construction across 20+ models"ccs","annotation", etc.) instead of positionalOutputs[0]— more resilient to API changesParseFragmentAnnotationhandles NL suffixes (-H2O,-NH3);BaseFragmentIdentifieruses first-dash strippingParameterHandlingModeswitches includedefault: throwValidateModelSpecificInputs