-
Notifications
You must be signed in to change notification settings - Fork 42
Ship XML documentation so consumers get tooltips, and point the README at the wiki #1162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
7a150d3
f026356
ae485ac
ca292eb
a4571c6
9406cb7
b79f3e4
bd84527
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,12 @@ | |
|
|
||
| namespace FlashLFQ | ||
| { | ||
| /// <summary> | ||
| /// An integrated elution peak for one identified species in one spectra file: the isotopic envelopes | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So in the default and recommended configuration this type is not an integrated peak -- its Describing it as the grouped isotopic envelopes, with the intensity being the apex envelope unless |
||
| /// that were grouped together, the resulting intensity, and how the peak was found. Peaks whose | ||
| /// <see cref="DetectionType"/> is MBR were transferred from another file by match-between-runs rather | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor, but Someone filtering |
||
| /// than identified in this one. | ||
| /// </summary> | ||
| public class ChromatographicPeak : IEquatable<ChromatographicPeak> | ||
| { | ||
| public double Intensity { get; private set; } | ||
|
|
@@ -38,9 +44,7 @@ public ChromatographicPeak(Identification id, SpectraFileInfo fileInfo, Detectio | |
| /// overloaded constructor for Isobaric_ambiguity peaks. In this case, the peak is identified by multiple identifications | ||
| /// </summary> | ||
| /// <param name="ids"></param> | ||
| /// <param name="isMbrPeak"></param> | ||
| /// <param name="fileInfo"></param> | ||
| /// <param name="randomRt"></param> | ||
| public ChromatographicPeak(List<Identification> ids, SpectraFileInfo fileInfo, DetectionType detectionType) | ||
| { | ||
| SplitRT = 0; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,12 @@ | |
|
|
||
| namespace FlashLFQ | ||
| { | ||
| /// <summary> | ||
| /// One peptide sequence's quantification across every spectra file: its intensity per file, how that | ||
| /// intensity was obtained per file, and the protein groups it belongs to. This is the roll-up of the | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
"Roll-up" reads as summed or otherwise combined, so a consumer may assume peptide intensity equals the total of its chromatographic peaks and find the numbers don't reconcile against QuantifiedPeaks. Stating that the per-file intensity comes from the single highest-intensity qualifying peak would match what the code does. |
||
| /// <see cref="ChromatographicPeak"/> objects sharing a sequence, and is what | ||
| /// QuantifiedPeptides.tsv is written from. | ||
| /// </summary> | ||
| public class Peptide | ||
| { | ||
| public readonly string Sequence; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things about the diagnostic policy this block sets out.
CS1574 and CS1572 aren't in the accounting. The comment presents a closed set of four -- CS1591/CS1573 suppressed, CS1570/CS1587 left visible -- but enabling
GenerateDocumentationFilealso turns on CS1574 and CS1572. CS1574 is load-bearing here: it is the diagnostic that surfaced all 14 crefs this PR degraded, yet it is never named. CS1572 is unhandled and does fire --MRSNoiseEstimator.cs:147andFlashLfqEngine.cs:2001-2002all carry<param>tags for parameters that do not exist (commented separately). Since the stated verification is "0 CS1570 / CS1574 / CS1587", anyone reproducing the non-incremental build locally will see CS1572 warnings the PR says nothing about, and won't be able to tell whether they were known or missed.The number on line 26 doesn't reconcile. It says roughly 200 CS1570/CS1587 "exist today", but the description reports the build at 0 for both and puts the total doc-comment warning count at 103 (60 in test/benchmark, 43 in shipped code). I can't make those three figures agree. This comment is what the next maintainer reads to decide whether a CS1570 they hit is a regression or expected background, so it's worth getting exact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π€ Both points hold, and the first is bigger than stated. Fixed in b79f3e4; I also corrected the PR description, with the edit noted at the bottom of it.
Seven diagnostics, not four.
GenerateDocumentationFileenables CS1570, CS1572, CS1573, CS1574, CS1587, CS1711, CS1734 and CS0419. The comment listed four. The ones you did not name fired too: CS1711 (typeparam tag with no such type parameter, 2), CS1734 (paramref with no such parameter, 1), and CS0419 (ambiguous cref, 24). Swept the tree β CS1572 39 β 0, CS1711 2 β 0, CS1734 1 β 0, CS1574 1 β 0. CS0419 left at 24, since those resolve to an overload: the link works and only the signature is arbitrary. The props comment now carries the full list.The CS1572 gap is not hypothetical β this branch creates two of them. On master,
FlashLfqEngine.cs:1995emits three CS1570s: the unescaped< start, end >aborts parsing of the entire block, so neitherpeaksInOneXICnorchromPeaksInThisSequenceis ever name-checked. The escaping fix makes the block well-formed, and both bad names then surface as CS1572. Net improvement, but it converts a listed diagnostic into an unlisted one, which is precisely the hole. Both retargeted tosharedPeakandchromPeaksInSharedPeak.Numbers. Measured against
upstream/masterwithGenerateDocumentationFile=trueforced on every project, non-incremental Release,NoWarncleared, counting unique file+line+column: CS1570 41 (29 shipped / 12 test), CS1587 54 (5 shipped / 49 test) β 95 total, split 34 shipped / 61 test-and-Development. So "roughly 200 of those exist today" was wrong in both tenses: 95 before this branch, 0 after. The description's 60 test-side figure was right; 43 shipped looks like transposed digits for 34, and part of the gap may be that the original count predates the master merge.That also reconciles your 14: shipped CS1574 measures 15 at current master. Fourteen are the ones degraded here; the fifteenth is
SdrfValidation.cs:38, which arrived with the merge after the verification ran.SdrfDocument.Resultsfails becauseResultsis inherited from the generic baseResultFile<TResult>, and Roslyn will not follow a qualified cref through a generic base. Naming the base explicitly fixes it.Figures are from macOS / .NET SDK 10.0.400 β worth a CI re-run before anyone leans on them.