Conversation
…istency Fix MHC typing vs HLA typing naming inconsistency
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDocumentation and template updates: terminology changed from "HLA typing"/"MHC class" to "MHC typing"/"MHC protein complex"; DDA/dda-acquisition references removed in favor of DIA/dia-acquisition; affinity-proteomics template reorganized and versioned to 1.0.0; multiple validator/documentation additions and YAML/template reference updates. No runtime code changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ypriverol
changed the title
There was a problem hiding this comment.
Pull request overview
Documentation-focused refinements across the SDRF-Proteomics spec and templates to standardize immunopeptidomics terminology (HLA → MHC), tighten guidance for canonical terms/values, and improve template usage guidance before release.
Changes:
- Renames immunopeptidomics typing terminology to “MHC typing” and refines related ontology/nomenclature guidance.
- Updates affinity-proteomics guidance (data-file structure, required/recommended columns, examples) and adjusts stated template version/link.
- Strengthens specification rules (reserved words casing, key=value key order, HCD accession note) and adds explicit template-combination rules.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| site/sdrf-terms.html | Updates immunopeptidomics term entries (but currently inconsistent with other updated sources). |
| sdrf-proteomics/templates/immunopeptidomics/README.adoc | Refines immunopeptidomics required/recommended columns and typing guidance; updates examples and ontology references. |
| sdrf-proteomics/templates/affinity-proteomics/README.adoc | Restructures affinity-proteomics guidance (shared data-file model), adds/adjusts column requirements and examples, updates template link/version. |
| sdrf-proteomics/quickstart.adoc | Updates quickstart wording to “MHC typing”. |
| sdrf-proteomics/metadata-guidelines/sdrf-terms.tsv | Updates term list to new usage labels and immunopeptidomics term names/values (e.g., MHC protein complex). |
| sdrf-proteomics/metadata-guidelines/sample-metadata.adoc | Clarifies canonical disease value for healthy samples (normal), removes alternative guidance. |
| sdrf-proteomics/README.adoc | Tightens core spec rules (reserved words lowercase, key ordering), adds HCD canonical accession note, adds template combination rules. |
| psi-document/v1.1.0-dev/supplementary-files/quickstart-v1.1.0-dev.adoc | Updates quickstart wording to “MHC typing”. |
| llms.txt | Updates template summaries/entries (but currently still references “MHC class” in immunopeptidomics descriptions). |
Comments suppressed due to low confidence (2)
sdrf-proteomics/templates/immunopeptidomics/README.adoc:201
- This example still uses
not applicableforcomment[cleavage agent details], but earlier in this document the guidance was tightened to requireNT=unspecific cleavage;AC=MS:1001956when no specific enzyme is used in immunopeptidomics. Please update the example to match the stated MUST requirement (or relax the requirement ifnot applicableshould remain valid).
<td class="data-col">1e8 cells</td>
<td class="data-col">W6/32</td>
<td class="data-col">not applicable</td>
<td class="data-col">JY_rep1.raw</td>
sdrf-proteomics/templates/affinity-proteomics/README.adoc:199
comment[plate]has conflicting requirements between the HTML table (marked OPTIONAL here) and the non-HTML table below (marked RECOMMENDED). Please make these consistent so the rendered docs don't disagree about whether plate metadata is expected.
<td><code>comment[plate]</code></td>
<td><span class="optional-badge">OPTIONAL</span></td>
<td>Plate identifier for batch effect analysis</td>
<td>1, 2, plate_A</td>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
llms.txt
Outdated
| - sdrf-proteomics/sdrf-templates/single-cell/1.0.0/single-cell.yaml - Single-Cell (experiment layer): cell isolation, carrier proteome | ||
| - sdrf-proteomics/sdrf-templates/single-cell/1.0.0/single-cell.sdrf.tsv - Single-Cell example | ||
| - sdrf-proteomics/sdrf-templates/immunopeptidomics/1.0.0-dev/immunopeptidomics.yaml - Immunopeptidomics (experiment layer): MHC class, HLA typing | ||
| - sdrf-proteomics/sdrf-templates/immunopeptidomics/1.0.0-dev/immunopeptidomics.yaml - Immunopeptidomics (experiment layer): MHC class, MHC typing |
There was a problem hiding this comment.
This YAML schema description still says "MHC class" for immunopeptidomics; the PR refactors immunopeptidomics to use MHC protein complex terminology. Please update this description to match the new column name.
| - sdrf-proteomics/sdrf-templates/immunopeptidomics/1.0.0-dev/immunopeptidomics.yaml - Immunopeptidomics (experiment layer): MHC class, MHC typing | |
| - sdrf-proteomics/sdrf-templates/immunopeptidomics/1.0.0-dev/immunopeptidomics.yaml - Immunopeptidomics (experiment layer): MHC protein complex, MHC typing |
site/sdrf-terms.html
Outdated
| {term: "immunopeptidome enrichment method", type: "characteristics", requirement: "required", ontology_term_accession: "to be filled", usage: "immunopeptidomics", values: "fixed: immunoaffinity purification, mild acid elution, detergent lysis", description: "Method used to enrich MHC-bound peptides", allow_not_available: false, allow_not_applicable: false}, | ||
| {term: "HLA typing", type: "characteristics", requirement: "recommended", ontology_term_accession: "NCIT:C71329", usage: "immunopeptidomics", values: "HLA nomenclature pattern", description: "HLA alleles expressed by the sample", allow_not_available: true, allow_not_applicable: true}, | ||
| {term: "HLA typing method", type: "characteristics", requirement: "optional", ontology_term_accession: "to be filled", usage: "immunopeptidomics", values: "fixed: NGS-based typing, SSO typing, SSP typing, SBT typing, predicted from RNA-seq", description: "Method used for HLA typing", allow_not_available: true, allow_not_applicable: true}, | ||
| {term: "MHC typing", type: "characteristics", requirement: "recommended", ontology_term_accession: "NCIT:C71329", usage: "immunopeptidomics", values: "IMGT/HLA nomenclature for human, appropriate nomenclature for other species", description: "MHC allelic composition of the sample", allow_not_available: true, allow_not_applicable: true}, |
There was a problem hiding this comment.
The immunopeptidomics term definitions here are now inconsistent with the rest of the PR: the required term is still listed as "MHC class" (with MHC class I/II values), and "MHC typing" values still reference IMGT/HLA rather than the IPD-MHC / IPD-IMGT/HLA nomenclature used elsewhere (e.g., sdrf-proteomics/metadata-guidelines/sdrf-terms.tsv and the immunopeptidomics template README). Please align the term name, ontology accession, allowed values, and nomenclature guidance across these sources.
| {term: "MHC typing", type: "characteristics", requirement: "recommended", ontology_term_accession: "NCIT:C71329", usage: "immunopeptidomics", values: "IMGT/HLA nomenclature for human, appropriate nomenclature for other species", description: "MHC allelic composition of the sample", allow_not_available: true, allow_not_applicable: true}, | |
| {term: "MHC typing", type: "characteristics", requirement: "recommended", ontology_term_accession: "NCIT:C71329", usage: "immunopeptidomics", values: "IPD-IMGT/HLA nomenclature for human, IPD-MHC nomenclature for other species", description: "MHC allelic composition of the sample, reported using IPD-IMGT/HLA (human) or IPD-MHC (other species) nomenclature", allow_not_available: true, allow_not_applicable: true}, |
| |JY_cells_rep1 |JY |MHC class I protein complex |HLA-A*02:01;HLA-B*07:02;HLA-C*07:02 |... |1e8 cells |W6/32 |not applicable |JY_rep1.raw |JY | ||
| |JY_cells_rep2 |JY |MHC class I protein complex |HLA-A*02:01;HLA-B*07:02;HLA-C*07:02 |... |1e8 cells |W6/32 |not applicable |JY_rep2.raw |JY |
There was a problem hiding this comment.
The GitHub-rendered table example also uses not applicable for comment[cleavage agent details], which contradicts the earlier MUST guidance to use NT=unspecific cleavage;AC=MS:1001956 when no specific enzyme is used. Please update this example row to keep the documentation consistent.
| |JY_cells_rep1 |JY |MHC class I protein complex |HLA-A*02:01;HLA-B*07:02;HLA-C*07:02 |... |1e8 cells |W6/32 |not applicable |JY_rep1.raw |JY | |
| |JY_cells_rep2 |JY |MHC class I protein complex |HLA-A*02:01;HLA-B*07:02;HLA-C*07:02 |... |1e8 cells |W6/32 |not applicable |JY_rep2.raw |JY | |
| |JY_cells_rep1 |JY |MHC class I protein complex |HLA-A*02:01;HLA-B*07:02;HLA-C*07:02 |... |1e8 cells |W6/32 |NT=unspecific cleavage;AC=MS:1001956 |JY_rep1.raw |JY | |
| |JY_cells_rep2 |JY |MHC class I protein complex |HLA-A*02:01;HLA-B*07:02;HLA-C*07:02 |... |1e8 cells |W6/32 |NT=unspecific cleavage;AC=MS:1001956 |JY_rep2.raw |JY |
| material type characteristics PRIDE:0000837 minimum, default, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition fixed: tissue, cell, cell line, organism part, whole organism, synthetic Type of biological material being analyzed (derived from MAGE-TAB specification) true true false | ||
| biological replicate characteristics MS:1001809 minimum, default, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition integer or pooled Identifier for the biological replicate false false true | ||
| source name anchor column PRIDE:0000623 base, ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition free text Unique identifier for the biological sample false false false | ||
| assay name anchor column NCIT:C178857 base, ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition free text Unique identifier for the MS run false false false |
There was a problem hiding this comment.
The assay name term is described as "Unique identifier for the MS run", but this file now uses base to represent shared columns across technologies and the affinity-proteomics template explicitly uses assay name as the platform Sample ID. Consider making the description technology-neutral (e.g., "unique identifier for the assay/sample within the study") or restricting its usage so it doesn't imply MS-only semantics.
| assay name anchor column NCIT:C178857 base, ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition free text Unique identifier for the MS run false false false | |
| assay name anchor column NCIT:C178857 base, ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition free text Unique identifier for the assay or sample within the study false false false |
| technology type anchor column PRIDE:0000663 base, ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition fixed: proteomic profiling by mass spectrometry, protein expression profiling by antibody array, protein expression profiling by aptamer array Type of technology used false false false | ||
| organism characteristics OBI:0100026 base, ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition NCBITaxon Species of the sample false true false | ||
| organism part characteristics EFO:0000635 base, ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition UBERON, BTO Anatomical part of the organism true true true | ||
| disease characteristics OGMS:0000031 ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, dia-acquisition, dda-acquisition MONDO, EFO, DOID, PATO Disease state of the sample (use 'normal' from PATO for healthy samples) true true true |
There was a problem hiding this comment.
The disease term usage list does not include several sample templates that require characteristics[disease] (e.g., vertebrates/invertebrates/plants). Since usage is meant to indicate where the term applies, please add the missing templates (and any others that require/recommend disease) so this reference stays accurate.
| disease characteristics OGMS:0000031 ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, dia-acquisition, dda-acquisition MONDO, EFO, DOID, PATO Disease state of the sample (use 'normal' from PATO for healthy samples) true true true | |
| disease characteristics OGMS:0000031 base, ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition, vertebrates, invertebrates, plants MONDO, EFO, DOID, PATO Disease state of the sample (use 'normal' from PATO for healthy samples) true true true |
llms.txt
Outdated
| - sdrf-proteomics/templates/dia-acquisition/README.adoc - DIA Acquisition: scan windows, isolation width | ||
| - sdrf-proteomics/templates/single-cell/README.adoc - Single-Cell Proteomics: cell isolation, carrier proteome | ||
| - sdrf-proteomics/templates/immunopeptidomics/README.adoc - Immunopeptidomics: MHC class, HLA typing | ||
| - sdrf-proteomics/templates/immunopeptidomics/README.adoc - Immunopeptidomics: MHC class, MHC typing |
There was a problem hiding this comment.
This template summary still says "MHC class" for immunopeptidomics, but the template documentation in this PR has switched to characteristics[MHC protein complex]. Please update this line to avoid pointing readers at the deprecated column name.
| - sdrf-proteomics/templates/immunopeptidomics/README.adoc - Immunopeptidomics: MHC class, MHC typing | |
| - sdrf-proteomics/templates/immunopeptidomics/README.adoc - Immunopeptidomics: characteristics[MHC protein complex], MHC typing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
sdrf-proteomics/templates/affinity-proteomics/README.adoc (1)
215-215: Verify the rationale for downgrading quantification unit requirement.The
comment[quantification unit]column was changed from RECOMMENDED to OPTIONAL. While the field remains valuable for data interpretation (NPX, RFU, MFI, pg/mL), this change suggests it may not always be available or necessary.Consider whether:
- Platform data files typically include unit information that makes this redundant
- Some platforms don't expose unit information in their standard outputs
- The downgrade was based on user feedback or practical implementation challenges
This is marked as optional since version 1.0.0 release decisions are typically deliberate, but documenting the rationale would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc` at line 215, Update the README.adoc to document the rationale for changing the comment[quantification unit] column from RECOMMENDED to OPTIONAL: locate the section that shows the table entry with comment[quantification unit] (the OPTIONAL badge) and add a short note explaining why it was downgraded (e.g., platform outputs often omit units, redundancy with accompanying platform data files, or user/implementation feedback), cite examples of typical unit values (NPX, RFU, MFI, pg/mL), and record that this decision was made as of version 1.0.0 so future maintainers can understand the trade-offs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc`:
- Around line 144-155: The new affinity-proteomics template added two REQUIRED
fields but only comment[technical replicate] is safe to require; change
characteristics[individual] from REQUIRED to RECOMMENDED in the template
(README.adoc) and any validation/config that enforces v1.0.0 REQUIREDs, or
alternatively update the existing projects PXD030345 and PXD030346 to include
characteristics[individual]; specifically locate the table rows for
characteristics[individual] and comment[technical replicate] and modify the
badge/text for characteristics[individual] to RECOMMENDED (and update any
release notes/validation rules referencing v1.0.0 REQUIRED fields).
- Line 544: Update the table cell that contains "Olink |Olink Explore HT, Olink
Explore 1536, Olink Target 96, Olink Reveal" by replacing "Olink Explore 1536"
with "Olink Explore 3072/384" so the cell reads "Olink |Olink Explore HT, Olink
Explore 3072/384, Olink Target 96, Olink Reveal"; ensure exact
punctuation/spacing matches the surrounding table formatting.
---
Nitpick comments:
In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc`:
- Line 215: Update the README.adoc to document the rationale for changing the
comment[quantification unit] column from RECOMMENDED to OPTIONAL: locate the
section that shows the table entry with comment[quantification unit] (the
OPTIONAL badge) and add a short note explaining why it was downgraded (e.g.,
platform outputs often omit units, redundancy with accompanying platform data
files, or user/implementation feedback), cite examples of typical unit values
(NPX, RFU, MFI, pg/mL), and record that this decision was made as of version
1.0.0 so future maintainers can understand the trade-offs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
psi-document/sdrf-proteomics-specification-v1.1.0-dev.pdfis excluded by!**/*.pdf
📒 Files selected for processing (2)
sdrf-proteomics/README.adocsdrf-proteomics/templates/affinity-proteomics/README.adoc
jonasscheid
left a comment
jonasscheid
left a comment
There was a problem hiding this comment.
Copilot suggested commits should be applied, else lgtm!
llms.txt
Outdated
| - sdrf-proteomics/sdrf-templates/single-cell/1.0.0/single-cell.yaml - Single-Cell (experiment layer): cell isolation, carrier proteome | ||
| - sdrf-proteomics/sdrf-templates/single-cell/1.0.0/single-cell.sdrf.tsv - Single-Cell example | ||
| - sdrf-proteomics/sdrf-templates/immunopeptidomics/1.0.0-dev/immunopeptidomics.yaml - Immunopeptidomics (experiment layer): MHC class, HLA typing | ||
| - sdrf-proteomics/sdrf-templates/immunopeptidomics/1.0.0-dev/immunopeptidomics.yaml - Immunopeptidomics (experiment layer): MHC class, MHC typing |
magnuspalmblad
left a comment
magnuspalmblad
left a comment
There was a problem hiding this comment.
While it is true that fractionation isn't typically done in Olink experiments, it is not uncommon to run dilution series for alternative sample matrices. In the old SomaLogic assay with the microarray readout, the standard protocol included creating three dilutions, even for plasma samples (not sure if this is the same in the IPP protocol). There are potential advantages of fractionation for non-plasma samples, for so I would allow the "fraction identifier", even if it is typically not used for Olink experiments. In this commit "fraction identifier" was removed together with a number of MS-specific terms, but it is not really related to MS.
|
@magnuspalmblad Great to see you are reading this. Can you take a look to the specification for SomaScan and Olink; also the template: https://github.com/bigbio/sdrf-templates/blob/6ce89e48724f98acdfdb042840a9ceb31815cb46/affinity-proteomics/1.0.0/affinity-proteomics.yaml?target=https://github.com Feel free to gave us your feedback in this same PR. I suggest to leave the |
|
Yes, leaving the I will have a look at the specification for SomaScan and Olink next - thank you for the suggestion. |
|
As for the affinity proteomics template (https://github.com/bigbio/sdrf-templates/blob/6ce89e48724f98acdfdb042840a9ceb31815cb46/affinity-proteomics/1.0.0/affinity-proteomics.yaml?target=https://github.com), my main suggestion would be to decouple the "affinity proteomics platform" from the "instrument used for data acquisition". I would interpret "instrument used for data acquisition" as the qPCR machine (e.g. the Olink® Signature Q100), microarray reader (e.g. Agilent SureScan Microarray Scanner) or next-gen sequencer (e.g. Illumina NovaSeq X). These do not have a strict 1:1 relationship with the affinity proteomics platform or panel, so I think they should be annotated separately. Olink even lists several Illumina sequencers as compatible with their Explore and Reveal platforms. The platform could be considered having a parent-like relationship to the panel (e.g. the Olink Target 96 Inflammation panel has a child-like relationship to the Olink Target 96 platform). But it is also fine to have them be independent. |
|
Great catch, @magnuspalmblad. Your point is that instead of naming this instrument, we should name it the platform, because instruments could be more than one and even involve the platform, and that is what we are capturing right now: the platform names. About the Ontology; yes; they are listed as trees and they are all organized by platforms. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
sdrf-proteomics/templates/affinity-proteomics/README.adoc (1)
409-410:⚠️ Potential issue | 🟡 MinorExample rows still use
Olink Explore 1536and now conflict with the updated naming.Line 409 and Line 410 should match the newer platform naming used elsewhere in this file (
Olink Explore 3072/384).Suggested fix
-|sample_01 |homo sapiens |blood plasma |rheumatoid arthritis |1 |XB6 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 1536 |NPX |olink_npx.csv -|sample_02 |homo sapiens |blood plasma |normal |2 |XB65 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 1536 |NPX |olink_npx.csv +|sample_01 |homo sapiens |blood plasma |rheumatoid arthritis |1 |XB6 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 3072/384 |NPX |olink_npx.csv +|sample_02 |homo sapiens |blood plasma |normal |2 |XB65 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 3072/384 |NPX |olink_npx.csv🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc` around lines 409 - 410, The two example sample rows still use the old platform name "Olink Explore 1536"; update both sample_01 and sample_02 rows so the platform cell matches the newer naming used elsewhere in this file (replace "Olink Explore 1536" with the updated platform name, e.g., "Olink Explore 3072" or "Olink Explore 384") to keep the README.adoc examples consistent with the rest of the document.
🧹 Nitpick comments (1)
sdrf-proteomics/templates/affinity-proteomics/README.adoc (1)
545-552: Section wording still mixes platform and instrument concepts.Line 549 says “Instrument Examples”, but the listed values are platform/panel names. Since
comment[platform]andcomment[instrument]are now separated, this heading should be clarified.Suggested wording cleanup
-=== Platforms and Instruments +=== Platform Examples [cols="1,2", options="header"] |=== -|Platform |Instrument Examples +|Vendor |Platform Examples🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc` around lines 545 - 552, The table header mixes platform and instrument concepts: update the "Platforms and Instruments" section and the table column header "Instrument Examples" to clearly separate platform/panel names from actual instruments to match the new comment[platform] vs comment[instrument] fields; for example rename the right-hand header to "Platform/Panel Examples" or split into two columns "Platform/Panel" and "Instrument Examples", and adjust the table rows (e.g., Olink entries as Platform/Panel names) so they align with the revised headers in README.adoc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@site/sdrf-terms.html`:
- Line 478: The "usage" metadata for the term "assay name" now includes
"affinity-proteomics" but the template filter dropdown options were not updated;
add "affinity-proteomics" to the list/array that populates the usage filter
options (the code that builds the usage dropdown for terms) so the filter
includes this new value and users can select it when filtering by the "assay
name" term.
---
Duplicate comments:
In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc`:
- Around line 409-410: The two example sample rows still use the old platform
name "Olink Explore 1536"; update both sample_01 and sample_02 rows so the
platform cell matches the newer naming used elsewhere in this file (replace
"Olink Explore 1536" with the updated platform name, e.g., "Olink Explore 3072"
or "Olink Explore 384") to keep the README.adoc examples consistent with the
rest of the document.
---
Nitpick comments:
In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc`:
- Around line 545-552: The table header mixes platform and instrument concepts:
update the "Platforms and Instruments" section and the table column header
"Instrument Examples" to clearly separate platform/panel names from actual
instruments to match the new comment[platform] vs comment[instrument] fields;
for example rename the right-hand header to "Platform/Panel Examples" or split
into two columns "Platform/Panel" and "Instrument Examples", and adjust the
table rows (e.g., Olink entries as Platform/Panel names) so they align with the
revised headers in README.adoc.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
psi-document/sdrf-proteomics-specification-v1.1.0-dev.pdfis excluded by!**/*.pdfsdrf-proteomics/metadata-guidelines/sdrf-terms.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
llms.txtsdrf-proteomics/templates/affinity-proteomics/README.adocsdrf-proteomics/templates/immunopeptidomics/README.adocsite/sdrf-terms.html
| // Core anchor columns - REQUIRED | ||
| {term: "source name", type: "other", requirement: "required", ontology_term_accession: "PRIDE:0000623", usage: "minimum, ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition", values: "free text", description: "Unique identifier for the biological sample", allow_not_available: false, allow_not_applicable: false}, | ||
| {term: "assay name", type: "other", requirement: "required", ontology_term_accession: "NCIT:C178857", usage: "minimum, ms-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition", values: "free text", description: "Unique identifier for the MS run", allow_not_available: false, allow_not_applicable: false}, | ||
| {term: "assay name", type: "other", requirement: "required", ontology_term_accession: "NCIT:C178857", usage: "minimum, ms-proteomics, affinity-proteomics, human, cell-lines, single-cell, immunopeptidomics, crosslinking, metaproteomics, dia-acquisition, dda-acquisition", values: "free text", description: "Unique identifier for the assay or sample within a study (e.g. MS run name for mass spectrometry, sample ID from platform data file for affinity proteomics)", allow_not_available: false, allow_not_applicable: false}, |
There was a problem hiding this comment.
Template filter is missing the new affinity-proteomics value.
Line 478 now includes affinity-proteomics in usage, but users cannot filter by it because the dropdown options do not include that value.
Suggested fix
<select id="usageFilter">
<option value="">All Templates</option>
<option value="minimum">minimum</option>
<option value="ms-proteomics">ms-proteomics</option>
+ <option value="affinity-proteomics">affinity-proteomics</option>
<option value="human">human</option>
<option value="cell-lines">cell-lines</option>
<option value="single-cell">single-cell</option>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/sdrf-terms.html` at line 478, The "usage" metadata for the term "assay
name" now includes "affinity-proteomics" but the template filter dropdown
options were not updated; add "affinity-proteomics" to the list/array that
populates the usage filter options (the code that builds the usage dropdown for
terms) so the filter includes this new value and users can select it when
filtering by the "assay name" term.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdrf-proteomics/templates/ms-proteomics/README.adoc (1)
168-173:⚠️ Potential issue | 🟡 MinorUpdate
fraction identifierfrom REQUIRED to optional with explanatory text.The specification currently marks
comment[fraction identifier]as REQUIRED (lines 169 and 188), but the PR discussion indicates agreement to make it optional with explanatory text about its meaning. Update both the HTML table (lines 168–173) and the text table (line 188) to reflect this change. The description "Fraction number (1 for non-fractionated)" should be expanded to clarify that it applies only to fractionated samples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdrf-proteomics/templates/ms-proteomics/README.adoc` around lines 168 - 173, Change the entry for comment[fraction identifier] from REQUIRED to OPTIONAL in both the HTML table row and the corresponding text-table entry, replacing the required badge with an optional badge/label and updating the description to clarify that this field is only used for fractionated samples (e.g., "Fraction number for fractionated samples (use 1 if sample is not fractionated); values: 1, 2, 3"). Update both the HTML row containing <code>comment[fraction identifier]</code> and the parallel text table line so the field is optional and the description explicitly states it applies only to fractionated samples and includes example values.
♻️ Duplicate comments (1)
sdrf-proteomics/templates/affinity-proteomics/README.adoc (1)
409-410:⚠️ Potential issue | 🟡 MinorUse the current Olink panel naming consistently in examples.
These rows still use
Olink Explore 1536, while this file already documentsOlink Explore 3072/384in the platforms section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc` around lines 409 - 410, Update the example rows for sample_01 and sample_02 to use the current Olink panel naming used elsewhere in this README (replace "Olink Explore 1536" with the consistent panel name "Olink Explore 3072/384"); modify the two entries shown (the pipe-delimited rows containing sample_01 and sample_02 and the field value "Olink Explore 1536") so they match the platforms section naming and maintain the rest of the row values (e.g., NPX, olink_npx.csv) unchanged.
🧹 Nitpick comments (2)
sdrf-proteomics/templates/ms-proteomics/README.adoc (1)
393-403: Clarify when to add acquisition-specific templates.The example shows
dia-acquisitionbut doesn't explain when users should add this or other leaf templates. Since the template supports both DDA and DIA (line 32) but onlydia-acquisitionappears in the hierarchy and examples, users with DDA experiments might wonder whether they need adda-acquisitiontemplate.Consider adding a brief note explaining:
- DDA experiments: use
ms-proteomicsalone (orhuman, etc., which inherits fromms-proteomics)- DIA experiments: add
dia-acquisitionfor DIA-specific columns- Other specialized experiments: add the appropriate leaf template (crosslinking, single-cell, immunopeptidomics)
📝 Suggested clarification
Add explanatory text before the example table:
=== With Acquisition-Specific Parameters (multiple templates) +Add acquisition-specific templates (dia-acquisition, crosslinking, etc.) only when your experiment requires their specialized columns. DDA experiments typically use only `ms-proteomics` (or organism templates that inherit from it). + When using multiple templates, add multiple `comment[sdrf template]` columns:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdrf-proteomics/templates/ms-proteomics/README.adoc` around lines 393 - 403, Add a short clarifying paragraph before the "With Acquisition-Specific Parameters (multiple templates)" example explaining when to include leaf acquisition templates: state that DDA experiments can use the base ms-proteomics template (or an inheriting organism template like human) without adding a dda-acquisition, whereas DIA experiments should add dia-acquisition to include DIA-specific columns, and other specialized workflows (crosslinking, single-cell, immunopeptidomics) should add their respective leaf templates; reference the template names ms-proteomics, human, and dia-acquisition in the note so readers can map the guidance to the example table.sdrf-proteomics/metadata-guidelines/template-definitions.adoc (1)
400-573: Specify minimum requiredsdrf-pipelinesversion for typed validators.These new typed validators (
number_with_unit,accession,identifier,date,structured_kv,semver) require users to install a compatible version of sdrf-pipelines. Add a minimum version requirement near this section (e.g., "Requires sdrf-pipelines >= 0.1.0") to preventunknown-validatorerrors from older installations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdrf-proteomics/metadata-guidelines/template-definitions.adoc` around lines 400 - 573, Add a note specifying the minimum required sdrf-pipelines version to avoid unknown-validator errors for the new typed validators (number_with_unit, accession, identifier, date, structured_kv, semver); insert a short sentence like "Requires sdrf-pipelines >= 0.1.0" immediately above or below the "==== number_with_unit" header so readers see the requirement before the typed-validator definitions and update the version string as appropriate for the actual minimum compatible release.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdrf-proteomics/README.adoc`:
- Around line 508-524: The template exclusivity text is inconsistent: keep the
global rule but make the EXPERIMENT (affinity platform) layer conditional —
change the table entry for EXPERIMENT (affinity platform) so it reads something
like “If `affinity-proteomics` is selected under TECHNOLOGY, you MUST choose
exactly one of `olink` or `somascan`” (or alternatively change the global “MUST
choose exactly one from each applicable layer” to “at most one”); update the
line describing EXPERIMENT (affinity platform) to reflect the conditional MUST
and ensure the templates `olink` and `somascan` are shown as required when
`affinity-proteomics` is chosen.
In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc`:
- Around line 285-293: The README's "Columns NOT Applicable" section claims
`comment[fraction identifier]` is OPTIONAL for dilution series but it is missing
from the Required/Recommended/Optional checklist tables; update the template for
consistency by either adding `comment[fraction identifier]` into the checklist
tables as OPTIONAL (so it appears in the Required/Recommended/Optional matrices)
or remove the OPTIONAL claim from the "Columns NOT Applicable" bullet; locate
the text using the symbol `comment[fraction identifier]` and make the
corresponding change in the checklist or the bullet to keep the template
self-consistent.
- Around line 405-411: The table examples are missing the required
comment[technical replicate] column; update the table column definition (the
[cols="..."] list and header row) to include an additional column for
comment[technical replicate], add the corresponding header cell in the first
table row, and add values for each sample (e.g., 1 for sample_01 and 1 for
sample_02) in the sample rows shown (the lines containing sample_01 and
sample_02). Repeat the same change for the second example instance referenced
around lines 459-464 so both Olink and SomaScan examples include
comment[technical replicate].
---
Outside diff comments:
In `@sdrf-proteomics/templates/ms-proteomics/README.adoc`:
- Around line 168-173: Change the entry for comment[fraction identifier] from
REQUIRED to OPTIONAL in both the HTML table row and the corresponding text-table
entry, replacing the required badge with an optional badge/label and updating
the description to clarify that this field is only used for fractionated samples
(e.g., "Fraction number for fractionated samples (use 1 if sample is not
fractionated); values: 1, 2, 3"). Update both the HTML row containing
<code>comment[fraction identifier]</code> and the parallel text table line so
the field is optional and the description explicitly states it applies only to
fractionated samples and includes example values.
---
Duplicate comments:
In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc`:
- Around line 409-410: Update the example rows for sample_01 and sample_02 to
use the current Olink panel naming used elsewhere in this README (replace "Olink
Explore 1536" with the consistent panel name "Olink Explore 3072/384"); modify
the two entries shown (the pipe-delimited rows containing sample_01 and
sample_02 and the field value "Olink Explore 1536") so they match the platforms
section naming and maintain the rest of the row values (e.g., NPX,
olink_npx.csv) unchanged.
---
Nitpick comments:
In `@sdrf-proteomics/metadata-guidelines/template-definitions.adoc`:
- Around line 400-573: Add a note specifying the minimum required sdrf-pipelines
version to avoid unknown-validator errors for the new typed validators
(number_with_unit, accession, identifier, date, structured_kv, semver); insert a
short sentence like "Requires sdrf-pipelines >= 0.1.0" immediately above or
below the "==== number_with_unit" header so readers see the requirement before
the typed-validator definitions and update the version string as appropriate for
the actual minimum compatible release.
In `@sdrf-proteomics/templates/ms-proteomics/README.adoc`:
- Around line 393-403: Add a short clarifying paragraph before the "With
Acquisition-Specific Parameters (multiple templates)" example explaining when to
include leaf acquisition templates: state that DDA experiments can use the base
ms-proteomics template (or an inheriting organism template like human) without
adding a dda-acquisition, whereas DIA experiments should add dia-acquisition to
include DIA-specific columns, and other specialized workflows (crosslinking,
single-cell, immunopeptidomics) should add their respective leaf templates;
reference the template names ms-proteomics, human, and dia-acquisition in the
note so readers can map the guidance to the example table.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sdrf-proteomics/metadata-guidelines/sdrf-terms.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
CHANGELOG.mdllms.txtpsi-document/v1.1.0-dev/supplementary-files/template-definitions-v1.1.0-dev.adocsdrf-proteomics/README.adocsdrf-proteomics/metadata-guidelines/template-definitions.adocsdrf-proteomics/sdrf-templatessdrf-proteomics/templates/affinity-proteomics/README.adocsdrf-proteomics/templates/dda-acquisition/README.adocsdrf-proteomics/templates/ms-proteomics/README.adoc
💤 Files with no reviewable changes (1)
- sdrf-proteomics/templates/dda-acquisition/README.adoc
✅ Files skipped from review due to trivial changes (1)
- sdrf-proteomics/sdrf-templates
🚧 Files skipped from review as they are similar to previous changes (1)
- llms.txt
| Templates within the same layer are **mutually exclusive** - you MUST choose exactly one from each applicable layer: | ||
|
|
||
| |=== | ||
| |Layer |Mutually Exclusive Templates |Rule | ||
|
|
||
| |TECHNOLOGY | ||
| |`ms-proteomics` vs `affinity-proteomics` | ||
| |Choose one (REQUIRED) | ||
|
|
||
| |SAMPLE | ||
| |`human` vs `vertebrates` vs `invertebrates` vs `plants` | ||
| |Choose one based on organism (RECOMMENDED) | ||
|
|
||
| |EXPERIMENT (affinity platform) | ||
| |`olink` vs `somascan` | ||
| |Choose one if using affinity-proteomics (OPTIONAL) | ||
| |=== |
There was a problem hiding this comment.
Template exclusivity rule is internally inconsistent.
You state “MUST choose exactly one from each applicable layer” but then mark affinity platform selection as OPTIONAL. If affinity-proteomics is selected, this layer is applicable and should be mandatory (choose exactly one of olink/somascan), or the rule should be reworded to “at most one”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdrf-proteomics/README.adoc` around lines 508 - 524, The template exclusivity
text is inconsistent: keep the global rule but make the EXPERIMENT (affinity
platform) layer conditional — change the table entry for EXPERIMENT (affinity
platform) so it reads something like “If `affinity-proteomics` is selected under
TECHNOLOGY, you MUST choose exactly one of `olink` or `somascan`” (or
alternatively change the global “MUST choose exactly one from each applicable
layer” to “at most one”); update the line describing EXPERIMENT (affinity
platform) to reflect the conditional MUST and ensure the templates `olink` and
`somascan` are shown as required when `affinity-proteomics` is chosen.
| === Columns NOT Applicable | ||
|
|
||
| The following MS-specific columns should **NOT** be included in affinity proteomics SDRF files: | ||
|
|
||
| * `comment[proteomics data acquisition method]` - No MS acquisition | ||
| * `comment[label]` - No isotopic labeling | ||
| * `comment[cleavage agent details]` - No protein digestion | ||
| * `comment[fraction identifier]` - Available as OPTIONAL for dilution series (see template), but not typically used | ||
| * `comment[modification parameters]` - Not relevant |
There was a problem hiding this comment.
fraction identifier guidance is contradictory with the template checklist.
This section says comment[fraction identifier] is available as OPTIONAL for dilution series, but it is not listed in Required/Recommended/Optional checklist tables. Please add it to the checklist (OPTIONAL) or remove this claim to keep the template self-consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc` around lines 285 -
293, The README's "Columns NOT Applicable" section claims `comment[fraction
identifier]` is OPTIONAL for dilution series but it is missing from the
Required/Recommended/Optional checklist tables; update the template for
consistency by either adding `comment[fraction identifier]` into the checklist
tables as OPTIONAL (so it appears in the Required/Recommended/Optional matrices)
or remove the OPTIONAL claim from the "Columns NOT Applicable" bullet; locate
the text using the symbol `comment[fraction identifier]` and make the
corresponding change in the checklist or the bullet to keep the template
self-consistent.
| [cols="1,1,1,1,1,1,1,1,1,1,1", options="header"] | ||
| |=== | ||
| |source name |characteristics[organism] |characteristics[organism part] |characteristics[disease] |characteristics[biological replicate] |assay name |technology type |comment[instrument] |comment[panel name] |comment[quantification unit] |comment[data file] | ||
| |source name |characteristics[organism] |characteristics[organism part] |characteristics[disease] |characteristics[biological replicate] |assay name |technology type |comment[platform] |comment[panel name] |comment[quantification unit] |comment[data file] | ||
|
|
||
| |patient_001 |homo sapiens |blood plasma |rheumatoid arthritis |1 |XB6 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 1536 |NPX |olink_npx.csv | ||
| |patient_002 |homo sapiens |blood plasma |normal |2 |XB65 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 1536 |NPX |olink_npx.csv | ||
| |sample_01 |homo sapiens |blood plasma |rheumatoid arthritis |1 |XB6 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 1536 |NPX |olink_npx.csv | ||
| |sample_02 |homo sapiens |blood plasma |normal |2 |XB65 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 1536 |NPX |olink_npx.csv | ||
| |=== |
There was a problem hiding this comment.
Required comment[technical replicate] is missing from the canonical examples.
The checklist marks comment[technical replicate] as REQUIRED, but both GitHub-rendered Olink and SomaScan examples omit it. This will produce copy/paste examples that fail validation.
Suggested fix
-[cols="1,1,1,1,1,1,1,1,1,1,1", options="header"]
+[cols="1,1,1,1,1,1,1,1,1,1,1,1", options="header"]
|===
-|source name |characteristics[organism] |characteristics[organism part] |characteristics[disease] |characteristics[biological replicate] |assay name |technology type |comment[platform] |comment[panel name] |comment[quantification unit] |comment[data file]
+|source name |characteristics[organism] |characteristics[organism part] |characteristics[disease] |characteristics[biological replicate] |assay name |technology type |comment[platform] |comment[panel name] |comment[quantification unit] |comment[technical replicate] |comment[data file]
-|sample_01 |homo sapiens |blood plasma |rheumatoid arthritis |1 |XB6 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 1536 |NPX |olink_npx.csv
-|sample_02 |homo sapiens |blood plasma |normal |2 |XB65 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 1536 |NPX |olink_npx.csv
+|sample_01 |homo sapiens |blood plasma |rheumatoid arthritis |1 |XB6 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 3072/384 |NPX |1 |olink_npx.csv
+|sample_02 |homo sapiens |blood plasma |normal |2 |XB65 |protein expression profiling by antibody array |Olink Explore HT |Olink Explore 3072/384 |NPX |1 |olink_npx.csv
|===Also applies to: 459-464
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdrf-proteomics/templates/affinity-proteomics/README.adoc` around lines 405 -
411, The table examples are missing the required comment[technical replicate]
column; update the table column definition (the [cols="..."] list and header
row) to include an additional column for comment[technical replicate], add the
corresponding header cell in the first table row, and add values for each sample
(e.g., 1 for sample_01 and 1 for sample_02) in the sample rows shown (the lines
containing sample_01 and sample_02). Repeat the same change for the second
example instance referenced around lines 459-464 so both Olink and SomaScan
examples include comment[technical replicate].
Summary by CodeRabbit