osprey: Collapsed the protein q-values into one field written per pass

pwiz_tools/Osprey/Osprey.Core/FdrEntry.cs

@@ -70,13 +70,28 @@ public class FdrEntry
         public double Score { get; set; }
         public double RunPrecursorQvalue { get; set; }
         public double RunPeptideQvalue { get; set; }
-        public double RunProteinQvalue { get; set; }
         public double ExperimentPrecursorQvalue { get; set; }
         public double ExperimentPeptideQvalue { get; set; }
-        public double ExperimentProteinQvalue { get; set; }
         public double Pep { get; set; }
         public string ModifiedSequence { get; set; }
 
+        /// <summary>
+        /// Picked-protein q-value for the protein group this entry's peptide maps to.
+        /// EXPERIMENT-scope in both passes: the first-pass protein FDR pools the detected
+        /// peptides over every file, computes one picked-protein FDR, and propagates one value
+        /// per <see cref="ModifiedSequence"/> to every entry in every file. There is no per-run
+        /// protein q anywhere in the pipeline.
+        ///
+        /// <para>Which PASS produced the value is encoded by the sidecar it is written to, not
+        /// by a second field - exactly as it is for the precursor and peptide q-values. The
+        /// 1st-pass sidecar carries the pass-1 value (the compaction protein-rescue gate reads
+        /// it back from there); the 2nd-pass sidecar carries the pass-2 value, patched in after
+        /// the second-pass protein FDR. Splitting the two passes across a
+        /// <c>RunProteinQvalue</c> / <c>ExperimentProteinQvalue</c> pair is what made a pass-1
+        /// value land in a pass-2 file and produced issue #4559.</para>
+        /// </summary>
+        public double ExperimentProteinQvalue { get; set; }
+
         /// <summary>
         /// The per-entry score the EXPERIMENT-scope competitions ranked this entry on (sidecar
         /// v4, issue #4522): max over the entry's rows across runs under the default
@@ -160,7 +175,6 @@ public FdrEntry()
         {
             RunPrecursorQvalue = 1.0;
             RunPeptideQvalue = 1.0;
-            RunProteinQvalue = 1.0;
             ExperimentPrecursorQvalue = 1.0;
             ExperimentPeptideQvalue = 1.0;
             ExperimentProteinQvalue = 1.0;
@@ -173,18 +187,17 @@ public FdrEntry()
         /// state a Stage 6 rescore target is left in for the 2nd pass to fill, and the state a
         /// fresh gap-fill stub is appended in.
         ///
-        /// <para>One method because the same eight assignments had been written out five
-        /// times - the rescore overlay, both gap-fill passes, and the rebuild-from-disk - and
-        /// the paths are compared against each other for byte identity. A field added to the
-        /// set in four places out of five is a divergence nothing would catch.</para>
+        /// <para>One method because the same assignments had been written out five times -
+        /// the rescore overlay, both gap-fill passes, and the rebuild-from-disk - and the
+        /// paths are compared against each other for byte identity. A field added to the set
+        /// in four places out of five is a divergence nothing would catch.</para>
         /// </summary>
         public void ResetScores()
         {
             Score = 0.0;
             ExperimentAggregateScore = 0.0;
             RunPrecursorQvalue = 1.0;
             RunPeptideQvalue = 1.0;
-            RunProteinQvalue = 1.0;
             ExperimentPrecursorQvalue = 1.0;
             ExperimentPeptideQvalue = 1.0;
             ExperimentProteinQvalue = 1.0;

pwiz_tools/Osprey/Osprey.FDR/FdrProjectionOutput.cs

@@ -32,7 +32,7 @@ namespace pwiz.Osprey.FDR
     /// FdrProjection struct-shrink S0). These are the outputs the lean
     /// <see cref="FdrProjection"/> no longer stores: the score pass hands each row's
     /// values to a per-pass <see cref="IFdrOutputSink"/> instead of overlaying them
-    /// onto the struct. <c>RunProteinQvalue</c> is NOT here -- it is produced by
+    /// onto the struct. <c>ExperimentProteinQvalue</c> is NOT here -- it is produced by
     /// first-pass protein FDR AFTER the score pass, not by the score pass.
     /// </summary>
     public readonly struct FdrQValues
@@ -106,7 +106,7 @@ public double EffectiveExperimentQvalue(FdrLevel level)
     /// each row's freshly computed <see cref="FdrQValues"/> + <see cref="FdrProjection.Score"/>
     /// to a caller-supplied sink. Both passes stream every row straight to the per-file
     /// <c>.fdr_scores.bin</c> sidecar (never stored -> 32 B resident): the 2nd pass writes
-    /// the final record, the 1st pass writes a phase-1 record whose <c>run_protein_qvalue</c>
+    /// the final record, the 1st pass writes a phase-1 record whose <c>experiment_protein_qvalue</c>
     /// first-pass protein FDR patches from disk afterward (issue #4355 struct-shrink S2; S1
     /// still kept a 16 B/row 1st-pass {RunPeptideQ, RunProteinQ} array, since removed). The
     /// sink also OWNS the tail <c>[COUNT]</c> tally (<see cref="Finish"/>),

pwiz_tools/Osprey/Osprey.FDR/ProteinFdr.cs

@@ -133,7 +133,7 @@ public ProteinFdrResult()
     /// The computed artifacts of a first-pass protein-FDR run, returned by
     /// <c>ProteinFdr.RunFirstPassProteinFdr</c> so the caller can log
     /// summary counts and emit the Stage-6 diagnostic dump WITHOUT recomputing
-    /// parsimony / FDR. The run has already propagated <c>RunProteinQvalue</c>
+    /// parsimony / FDR. The run has already propagated <c>ExperimentProteinQvalue</c>
     /// onto the stubs; these are the same intermediate objects it used.
     /// </summary>
     public class FirstPassProteinFdrResult
@@ -175,9 +175,9 @@ public FirstPassProteinFdrResult(
     /// (detected-gate + best max-score / min-q) are order-independent and a modified sequence
     /// maps to a single target/decoy label, so any streaming order reproduces the resident
     /// <see cref="ProteinFdr.CollectBestPeptideScores(IList{KeyValuePair{string, List{FdrEntry}}})"/>
-    /// path byte-identically. The caller then patches each entry's <c>run_protein_qvalue</c>
+    /// path byte-identically. The caller then patches each entry's <c>experiment_protein_qvalue</c>
     /// onto the sidecar from <see cref="FirstPassProteinFdrResult.ProteinFdr"/>'s
-    /// <c>PeptideQvalues</c> (replacing the resident <c>PropagateRunProteinQvalues</c> +
+    /// <c>PeptideQvalues</c> (replacing the resident <c>PropagateProteinQvalues</c> +
     /// phase-2 patch with one streaming pass).
     /// </summary>
     public sealed class FirstPassProteinFdrAccumulator
@@ -224,7 +224,7 @@ public void Add(string modifiedSequence, bool isDecoy, double score, double runP
         /// <summary>
         /// Run the identical parsimony + picked-protein FDR the buffer path runs and return
         /// the artifacts (the caller logs summary counts + patches the sidecar's
-        /// <c>run_protein_qvalue</c> from <see cref="ProteinFdrResult.PeptideQvalues"/>). The
+        /// <c>experiment_protein_qvalue</c> from <see cref="ProteinFdrResult.PeptideQvalues"/>). The
         /// cross-impl best-peptide-scores dump fires here, at the same point the resident
         /// <see cref="ProteinFdr.CollectBestPeptideScores(IList{KeyValuePair{string, List{FdrEntry}}})"/>
         /// emits it (after the reduction is complete).
@@ -248,7 +248,7 @@ public FirstPassProteinFdrResult Finish(IList<LibraryEntry> fullLibrary, OspreyC
     /// by <see cref="ProteinFdrEngine.RunSecondPass"/> so the Tasks-layer caller can
     /// emit the Stage-7 detected-peptides and protein-FDR diagnostic dumps (and the
     /// <c>Stage7ProteinFdrOnly</c> early-exit decision) WITHOUT recomputing parsimony
-    /// / FDR. The run has already propagated <c>RunProteinQvalue</c> and
+    /// / FDR. The run has already propagated <c>ExperimentProteinQvalue</c> and
     /// <c>ExperimentProteinQvalue</c> onto the stubs; these are the same intermediate
     /// objects it used.
     /// </summary>
@@ -888,13 +888,16 @@ public static Dictionary<string, PeptideScore> CollectBestPeptideScores(
         }
 
         /// <summary>
-        /// Propagate protein q-values to FdrEntry stubs.
+        /// Propagate protein q-values to FdrEntry stubs. Assigns by
+        /// <see cref="FdrEntry.ModifiedSequence"/> over EVERY entry, whether or not it passed
+        /// anything - a peptide's protein q is a property of the peptide, so an entry that lost
+        /// its own competition still carries its protein's value. Both passes call this; which
+        /// pass a value came from is recorded by the sidecar it is written to, not by writing a
+        /// second field (see <see cref="FdrEntry.ExperimentProteinQvalue"/>).
         /// </summary>
         public static void PropagateProteinQvalues(
             IList<KeyValuePair<string, List<FdrEntry>>> perFileEntries,
-            ProteinFdrResult proteinFdr,
-            bool setRun,
-            bool setExperiment)
+            ProteinFdrResult proteinFdr)
         {
             foreach (var file in perFileEntries)
             {
@@ -903,31 +906,32 @@ public static void PropagateProteinQvalues(
                     double q;
                     if (!proteinFdr.PeptideQvalues.TryGetValue(entry.ModifiedSequence, out q))
                         q = 1.0;
-                    if (setRun)
-                        entry.RunProteinQvalue = q;
-                    if (setExperiment)
-                        entry.ExperimentProteinQvalue = q;
+                    entry.ExperimentProteinQvalue = q;
                 }
             }
         }
 
         /// <summary>
         /// First-pass protein FDR: build parsimony from peptides passing peptide-level
         /// run FDR, run picked-protein FDR at <see cref="OspreyConfig.RunFdr"/> (1x Savitski
-        /// gate), and write the resulting q-values into <see cref="FdrEntry.RunProteinQvalue"/>
-        /// on every stub. Mirrors Rust <c>pipeline.rs::run_analysis</c> first-pass block
-        /// (around line 4292). Caller is responsible for any logging, dump diagnostics,
-        /// and downstream consumption.
+        /// gate), and write the resulting q-values into
+        /// <see cref="FdrEntry.ExperimentProteinQvalue"/> on every stub. Mirrors Rust
+        /// <c>pipeline.rs::run_analysis</c> first-pass block (around line 4292). Caller is
+        /// responsible for any logging, dump diagnostics, and downstream consumption.
+        ///
+        /// The result is EXPERIMENT-scope even though its detected-peptide gate is run-level:
+        /// the detected set is pooled over every file, one picked-protein FDR runs, and one
+        /// value per peptide reaches every entry in every file. What makes it the FIRST-PASS
+        /// value is when it runs, not what it is scoped to.
         ///
         /// Used by FirstPassFdrTask for the in-process pipeline (runs after first-pass FDR,
         /// before compaction) and by PerFileRescoreTask for the <c>--task SecondPassFDR</c>
         /// rehydration path (runs after sidecar load, before compaction) so the protein-
-        /// rescue branch of compaction has fresh <c>RunProteinQvalue</c> values matching
-        /// what Rust computes inline. Without it, the rehydrated C# pipeline used only
-        /// the <c>RunProteinQvalue</c> values stored in the 1st-pass FDR sidecar; for
-        /// single-file <c>--task SecondPassFDR</c> runs that left 19 peptides outside Rust's
-        /// post-compaction detected set on Stellar Single, causing a 1-protein delta in
-        /// Stage 7 picked-protein output.
+        /// rescue branch of compaction has fresh values matching what Rust computes inline.
+        /// Without it, the rehydrated C# pipeline used only the values stored in the 1st-pass
+        /// FDR sidecar; for single-file <c>--task SecondPassFDR</c> runs that left 19 peptides
+        /// outside Rust's post-compaction detected set on Stellar Single, causing a 1-protein
+        /// delta in Stage 7 picked-protein output.
         /// </summary>
         public static FirstPassProteinFdrResult RunFirstPassProteinFdr(
             IList<KeyValuePair<string, List<FdrEntry>>> perFileEntries,
@@ -951,10 +955,9 @@ public static FirstPassProteinFdrResult RunFirstPassProteinFdr(
             var bestScores = CollectBestPeptideScores(perFileEntries);
             var proteinFdr = ComputeProteinFdr(parsimony, bestScores, config.RunFdr);
 
-            // Set RunProteinQvalue ONLY. ExperimentProteinQvalue is set by the
-            // post-output Stage 7 second-pass protein FDR (Rust's second-pass).
-            PropagateProteinQvalues(perFileEntries, proteinFdr,
-                setRun: true, setExperiment: false);
+            // The Stage 7 second-pass protein FDR overwrites this with its own value; the
+            // pass-1 value is captured on the way past, into the 1st-pass sidecar.
+            PropagateProteinQvalues(perFileEntries, proteinFdr);
 
             // Return the computed artifacts so the caller can log summary counts
             // and emit the Stage-6 diagnostic dump without recomputing them.

pwiz_tools/Osprey/Osprey.FDR/ProteinFdrEngine.cs

@@ -50,7 +50,7 @@ public static class ProteinFdrEngine
         /// First-pass protein FDR (pre-Stage-6, full pre-compaction pool): builds
         /// parsimony from peptides passing peptide-level run FDR, runs picked-protein
         /// FDR at <see cref="OspreyConfig.RunFdr"/>, and writes
-        /// <see cref="FdrEntry.RunProteinQvalue"/> on every stub. The pure computation
+        /// <see cref="FdrEntry.ExperimentProteinQvalue"/> on every stub. The pure computation
         /// lives in <c>ProteinFdr.RunFirstPassProteinFdr</c>; this adds the
         /// summary logging and returns the artifacts so the Tasks facade can emit the
         /// Stage-6 diagnostic dump + <c>ProteinFdrOnly</c> early-exit WITHOUT
@@ -103,9 +103,10 @@ public static void LogFirstPassSummary(
         /// Second-pass / run-wide protein FDR (SecondPassFDR, post Stage-6): collects
         /// best peptide scores, gates the detected-peptide set on experiment-level
         /// q-value, builds parsimony, runs picked-protein FDR at
-        /// <see cref="OspreyConfig.RunFdr"/>, and propagates both
-        /// <see cref="FdrEntry.RunProteinQvalue"/> and
-        /// <see cref="FdrEntry.ExperimentProteinQvalue"/> onto every stub. Logs
+        /// <see cref="OspreyConfig.RunFdr"/>, and propagates
+        /// <see cref="FdrEntry.ExperimentProteinQvalue"/> onto every stub, OVERWRITING the
+        /// first-pass value the Stage-5 run left there (the pass-1 value has already been
+        /// captured into the 1st-pass sidecar by then). Logs
         /// summary counts via <paramref name="logInfo"/> (which may be null for a
         /// silent run, like <c>RunFirstPass</c>) and returns the parsimony /
         /// FDR artifacts so the Tasks facade can emit the Stage-7 detected-peptides
@@ -196,7 +197,7 @@ public static SecondPassProteinFdrResult RunSecondPass(
             // the dumps do not read, so emitting the dump after propagation is
             // output-invariant -- and matches the first-pass ordering, where
             // RunFirstPassProteinFdr likewise propagates before the facade dump.
-            ProteinFdr.PropagateProteinQvalues(perFileEntries, proteinFdr, true, true);
+            ProteinFdr.PropagateProteinQvalues(perFileEntries, proteinFdr);
 
             return new SecondPassProteinFdrResult(detectedPeptides, parsimony, proteinFdr);
         }

pwiz_tools/Osprey/Osprey.FDR/Reconciliation/ConsensusRts.cs

@@ -246,7 +246,7 @@ private static bool Qualifies(FdrEntry entry, double consensusFdr, double protei
             if (entry.RunPrecursorQvalue > consensusFdr)
                 return false;
             return entry.RunPeptideQvalue <= consensusFdr ||
-                   (proteinFdrThreshold > 0.0 && entry.RunProteinQvalue <= proteinFdrThreshold);
+                   (proteinFdrThreshold > 0.0 && entry.ExperimentProteinQvalue <= proteinFdrThreshold);
         }
 
         private static bool IsFinite(double d)

pwiz_tools/Osprey/Osprey.IO/FdrScoreRecord.cs

@@ -25,12 +25,12 @@ namespace pwiz.Osprey.IO
 {
     /// <summary>
     /// One <c>.fdr_scores.bin</c> record's payload (entry_id + SVM score + 4 q-values +
-    /// PEP + run_protein_qvalue + experiment_aggregate_score), decoupled from any resident
+    /// PEP + experiment_protein_qvalue + experiment_aggregate_score), decoupled from any resident
     /// buffer (issue #4355 struct-shrink S0). Since the lean <c>FdrProjection</c> no longer
     /// carries the q-value outputs, the projection sidecar writers assemble records of this
     /// shape -- the 1st pass from the lean row's <c>EntryId</c>/<c>Score</c> plus the parallel
     /// outputs array, the 2nd pass from the streamed q-values plus the survivor's
-    /// <c>RunProteinQvalue</c> lookup -- and hand them to
+    /// <c>ExperimentProteinQvalue</c> lookup -- and hand them to
     /// <see cref="FdrScoresSidecar.Write(string, System.Collections.Generic.IReadOnlyList{FdrScoreRecord}, FdrScoresSidecar.Pass)"/>.
     /// The 68-byte byte layout stays single-sourced through
     /// <c>FdrScoresSidecar.WriteRecord</c>.
@@ -44,7 +44,7 @@ public readonly struct FdrScoreRecord
         public readonly double ExperimentPrecursorQvalue;
         public readonly double ExperimentPeptideQvalue;
         public readonly double Pep;
-        public readonly double RunProteinQvalue;
+        public readonly double ExperimentProteinQvalue;
 
         /// <summary>
         /// The per-entry score the EXPERIMENT-scope competitions ranked this entry on, beside
@@ -75,7 +75,7 @@ public FdrScoreRecord(
             uint entryId, double score,
             double runPrecursorQvalue, double runPeptideQvalue,
             double experimentPrecursorQvalue, double experimentPeptideQvalue,
-            double pep, double runProteinQvalue, double experimentAggregateScore)
+            double pep, double experimentProteinQvalue, double experimentAggregateScore)
         {
             EntryId = entryId;
             Score = score;
@@ -84,7 +84,7 @@ public FdrScoreRecord(
             ExperimentPrecursorQvalue = experimentPrecursorQvalue;
             ExperimentPeptideQvalue = experimentPeptideQvalue;
             Pep = pep;
-            RunProteinQvalue = runProteinQvalue;
+            ExperimentProteinQvalue = experimentProteinQvalue;
             ExperimentAggregateScore = experimentAggregateScore;
         }
     }