Skip to content

feat(scATAC): add HTAN Phase 2 scATAC-seq module (schema + tests) - #194

Merged
aditigopalan merged 16 commits into
mainfrom
RFC-scATACseq
Jul 24, 2026
Merged

feat(scATAC): add HTAN Phase 2 scATAC-seq module (schema + tests)#194
aditigopalan merged 16 commits into
mainfrom
RFC-scATACseq

Conversation

@aditigopalan

@aditigopalan aditigopalan commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

feat(scATAC): add scATAC-seq module

Summary

Adds a new scATAC-seq LinkML module implementing the HTAN Phase 2 single-cell ATAC-seq RFC (

Noting that the final version of the RFC is stored on the H2 RFC GoogleDrive.

This PR also adds a shared single-cell layer to the existing Sequencing base module and refactors scRNA-seq to consume it, so common single-cell attributes are defined once instead of duplicated (see Design decisions).

Fixes #151

This PR contains source schemas + tests + registration only. No generated artifacts are included (see below).

What's included

New scATAC-seq module (modules/scATAC-seq/domains/)

  • level_1.yamlscATACLevel1: raw sequencing files (fastq/fastq.gz), single-nucleus preparation, barcode/read structure, and transposition metadata
  • level_2.yamlscATACLevel2: aligned data (bam/cram) and alignment/QC metrics
  • level_3_4.yamlscATACLevel3and4: peak-by-cell matrices and fragment files (h5ad/bed) with chromatin-accessibility metrics and AnnData 0.1 compliance
  • scatac_seq.yaml – container importing all levels
  • scaffolding: Makefile, python package (htan_scatac_seq), README.md
  • tests/test_scatac_seq.py (103 tests)

Shared single-cell layer (added to modules/Sequencing/domains/sequencing.yaml)

  • SingleCellLevel1Attributes (is_a BaseSequencingLevel1Attributes) – shared upstream prep attrs (single-cell isolation, dissociation, cryopreservation, nucleic-acid source, library construction) + their enums
  • AnnDataComplianceMixin – the AnnData 0.1 / CellxGene pair, applied to Level 3/4 classes via mixins:
  • covered by modules/Sequencing/tests/test_single_cell_layer.py

scRNA-seq refactorscRNALevel1 now is_a SingleCellLevel1Attributes; scRNALevel3and4 applies AnnDataComplianceMixin; duplicated enums/attrs removed. Tests updated to read inherited/mixin slots via induced slots.

Registration & docs – root Makefile (MODULES + format/test paths); CLAUDE.md inheritance section + review rules updated for the single-cell layer.

Design decisions

  • Inheritance:
CoreFileAttributes                          FILENAME, FILE_FORMAT, HTAN_DATA_FILE_ID, HTAN_PARENT_ID
│
└── BaseSequencingAttributes                CHECKSUM
    │
    └── BaseSequencingLevel1Attributes      LIBRARY_LAYOUT, SEQUENCING_PLATFORM, …
        │                                   ┌──────── defined in Sequencing base ────────┐
        ├── SingleCellLevel1Attributes      isolation, dissociation, cryopreservation,
        │   │                               nucleic-acid source, library construction
        │   ├── scRNALevel1                 (+ RT primer, spike-in, read indicator)   [scRNA-seq]
        │   └── scATACLevel1                (+ transposition, nuclei barcode, reads…) [scATAC-seq]
        │
        └── BaseSequencingLevel2Attributes  GENOMIC_REFERENCE, WORKFLOW_*, …
            │
            ├── scRNALevel2   scATACLevel2   BulkWESLevel2
            │
            └── BaseSequencingLevel3Attributes
                │
                ├── scRNALevel3and4    ⊕ AnnDataComplianceMixin   [scRNA-seq]
                ├── scATACLevel3and4   ⊕ AnnDataComplianceMixin   [scATAC-seq]
                └── BulkWESLevel3

  is_a chain: solid branches (single inheritance); the L1→L2→L3 spine is pre-existing.
  ⊕ = mixed in via `mixins:` (AnnDataComplianceMixin → ANNDATA_SCHEMA_VERSION,
      ANNDATA_STRUCTURE_VALIDATED), orthogonal to the is_a chain.
  SingleCellLevel1Attributes is_a BaseSequencingLevel1Attributes, so single-cell
  Level 1 never skips the chain. WES has no single-cell layer.
  • Shared single-cell layer over duplication: genuinely shared single-cell prep attributes/enums live once in the Sequencing base module and are inherited by both scRNA-seq and scATAC-seq. This follows the repo's existing precedent of keeping shared sequencing concepts in that base (e.g. LibraryLayoutEnum, GenomicReferenceEnum), now that scATAC is the second single-cell consumer.
  • Biology drives what's shared: only upstream tissue-to-cell/nucleus prep sits on the shared base class. Reverse transcription and spike-ins are RNA-workflow concepts, so their enums are centralized (to avoid duplication) but the slots are declared per assay. REVERSE_TRANSCRIPTION_PRIMER/SPIKE_IN are retained on scATAC per the RFC, but flagged for community review since ATAC has no reverse-transcription step.
  • Per-level file constraints: FILE_FORMAT/FILENAME are re-specified at each level with format-specific patterns — fastq/fastq.gz @ L1, bam/cram @ L2, h5ad/bed @ L3/4.
  • Container inlining: container slots are inlined: true because their ranges are identifier-bearing classes.

Not in this PR (by design)

  • Generated artifacts (src/*/datamodel/*.py, JSON_Schemas/*.json) are produced in a separate downstream make modules-gen PR, per repo convention.
  • Cross-level sanity checks the RFC assigns to the validatorDUPLICATE_READ_PAIRS, CHIMERIC_READ_PAIRS, UNMAPPED_READ_PAIRSTOTAL_READ_PAIRS (a Level 1 slot), and ON_TARGET_FRAGMENTS, PEAK_REGION_FRAGMENTSPASSED_FILTERS (a Level 2 slot) — reference slots in other levels and are not expressible in the flat Synapse JSON Schema. They are documented in the slot descriptions instead.
  • The RFC .xlsx attribute workbook is intentionally kept out of the repo.

Testing

  • Full module suite (poetry run pytest modules/) → 321 passed, 1 skipped (nothing regressed in scRNA-seq or elsewhere after the refactor).
  • scATAC-seq (103) / SingleCell / scRNA-seq suites all green: schema load, container wiring + inlining, inheritance/level chain, the shared single-cell layer + AnnData mixin, required + optional slot coverage, title+description completeness, numeric bounds, file-format/filename patterns, enums alphabetical + described, the paired-end conditional rule, and valid/invalid instance loads per class.

New module modules/scATAC-seq/ implementing the scATAC-seq RFC (data model
v1.5.0), parallel to scRNA-seq:

- level_1.yaml (scATACLevel1 is_a BaseSequencingLevel1Attributes): raw fastq
  files, single-nucleus prep, barcode/read enums, transposition reaction
- level_2.yaml (scATACLevel2 is_a BaseSequencingLevel2Attributes): bam/cram,
  alignment QC metrics; AVERAGE_INSERT_SIZE gated on paired-end layout
- level_3_4.yaml (scATACLevel3and4 is_a BaseSequencingLevel3Attributes):
  h5ad/bed, peak/fragment metrics, AnnData 0.1 compliance
- scatac_seq.yaml container, Makefile, README
- tests/test_scatac_seq.py (inheritance, required/optional, patterns, enums,
  numeric bounds, conditional rule, valid/invalid instances)
- register module in root Makefile (MODULES + test paths)

Cross-level sanity checks (e.g. DUPLICATE_READ_PAIRS <= TOTAL_READ_PAIRS)
documented in slot descriptions; not enforceable in flat Synapse JSON Schema.
Generated Python/JSON artifacts intentionally excluded (downstream PR).
github-actions[bot]

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

Define the attributes shared by single-cell sequencing assays once in the
Sequencing base module and inherit them, rather than duplicating across
scRNA-seq and scATAC-seq.

Added to modules/Sequencing/domains/sequencing.yaml:
- SingleCellLevel1Attributes (is_a BaseSequencingLevel1Attributes): shared
  upstream prep attrs (single-cell isolation, dissociation, cryopreservation,
  nucleic-acid source, library construction) + their enums
- AnnDataComplianceMixin: ANNDATA_SCHEMA_VERSION / ANNDATA_STRUCTURE_VALIDATED,
  applied via mixins: to the Level 3/4 classes
- reverse-transcription / spike-in enums centralized here; slots stay per-assay
  (RNA-workflow concepts; scATAC inclusion under community review)

Consumers:
- scRNALevel1 and scATACLevel1 now is_a SingleCellLevel1Attributes; level chain
  preserved (SingleCellLevel1Attributes is_a BaseSequencingLevel1Attributes)
- scRNALevel3and4 and scATACLevel3and4 apply AnnDataComplianceMixin
- affected tests read inherited/mixin slots via induced slots

Kept in the existing Sequencing base module rather than a standalone module:
same inheritance semantics and identical flat Synapse output, fewer moving
parts for the current two consumers. Shared-layer coverage lives in
modules/Sequencing/tests/test_single_cell_layer.py.

Full module suite: 320 passed, 1 skipped.
@github-actions
github-actions Bot dismissed their stale review July 22, 2026 16:04

Superseded by updated review on commit a48b27a

github-actions[bot]

This comment was marked as outdated.

Auto-generated Python classes from LinkML schema updates.

**Auto-generated by GitHub Actions workflow**
[skip ci]
@github-actions

This comment was marked as outdated.

- README.md: add scATAC-seq module section + project-structure entry; note the
  shared single-cell layer (SingleCellLevel1Attributes + AnnDataComplianceMixin)
  under the Sequencing and scRNA-seq module sections
- modules/Sequencing/README.md: document the shared single-cell layer

Generated per-module docs (docs/*.md, docs/csv/*) are produced downstream via
gen-doc and are not hand-edited here.
@github-actions
github-actions Bot dismissed their stale review July 22, 2026 16:12

Superseded by updated review on commit bec6ef3

github-actions[bot]
github-actions Bot previously approved these changes Jul 22, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTAN Schema Review — ✅ Approved

Automated review by htan-claude · commit bec6ef3170b0c638ba2f1176538769b9a4d0e167

Solid PR overall — the inheritance design is clean and the test suite is thorough, but there are a few issues to resolve before merge, including one blocking biological correctness concern and a missing title field on the container slots.

Caution

2 blocking issues must be resolved before merge

Files Changed

  • modules/scATAC-seq/domains/level_1.yaml
  • modules/scATAC-seq/domains/level_2.yaml
  • modules/scATAC-seq/domains/level_3_4.yaml
  • modules/scATAC-seq/domains/scatac_seq.yaml
  • modules/Sequencing/domains/sequencing.yaml
  • modules/scRNA-seq/domains/scrna_seq.yaml (refactor)
  • modules/scRNA-seq/tests/test_scrna_seq.py
  • modules/scATAC-seq/tests/test_scatac_seq.py
  • modules/Sequencing/tests/test_single_cell_layer.py
  • modules/SingleCell/ (new scaffolding)
  • Root Makefile, CLAUDE.md

Checklist Results

Check Result Notes
Inheritance correctness PASS All five changed/new classes chain correctly to CoreFileAttributes
Inlining of nested objects PASS All three container slots carry inlined: true with identifier-bearing ranges
Slot completeness (range, title, description) FAIL Container slots in scatac_seq.yaml missing title; MEDIAN_GENES_PER_CELL semantically wrong for ATAC
Enum integrity (alphabetical, descriptions) PASS All 10 new enums are alphabetical and fully described
Generated artifacts N/A Excluded by design per PR description

Findings

Blocking

  • modules/scATAC-seq/domains/level_1.yamlREVERSE_TRANSCRIPTION_PRIMER and SPIKE_IN marked required: true on an ATAC class (structural + biological)
    scATAC-seq involves Tn5 transposition of genomic DNA — there is no reverse-transcription step and ERCC spike-ins are an RNA-seq construct. Marking these required: true means every valid scATAC-seq submission will fail schema validation, because there is no scientifically meaningful value a submitter can provide. The PR description acknowledges this is "under community review," but from a structural standpoint, shipping them as required: true is a breaking constraint. Change both to required: false (and update their descriptions to clarify ATAC applicability) until the RFC community review explicitly endorses their presence and semantics for ATAC.

  • modules/scATAC-seq/domains/scatac_seq.yaml — Container slots level1_data, level2_data, level3_4_data missing title (structural + biological + coverage)
    The repo's slot-completeness rule requires every slot to carry range, title, and description. All three container slots on scATACseqData have range and description but no title field. Add a title to each (e.g., title: "Level 1 scATAC-seq Data", title: "Level 2 scATAC-seq Data", title: "Level 3/4 scATAC-seq Data").

Warnings

  • modules/scATAC-seq/domains/level_2.yamlMEDIAN_GENES_PER_CELL is semantically incorrect for scATAC-seq (biological)
    The slot title and description reference "genes with detected gene expression per cell," which is an RNA-seq concept. At scATAC-seq Level 2 (aligned BAM), gene-level expression is not computed — chromatin accessibility is. This slot appears to have been copied from scRNA-seq metadata and will confuse submitters or lead to corrupted metadata. If the intent is a cell-QC metric derived from accessibility (e.g., median fragments overlapping gene bodies), rename the slot and rewrite its description to reflect the ATAC-seq interpretation.

  • modules/Sequencing/tests/test_single_cell_layer.py — No valid/invalid instance-level tests for SingleCellLevel1Attributes or AnnDataComplianceMixin (coverage)
    The coverage rules expect at least one valid-instance load and one invalid-instance load per new class. The test file covers existence, is_a, required/optional slots, and enum ordering, but includes no instance-level load tests for these two new classes. If direct instantiation is not meaningful for abstract/mixin types in this framework, add a comment in the test file explaining the omission; otherwise add at least one valid and one invalid instance test exercised through a concrete subclass.

Informational

  • modules/Sequencing/domains/sequencing.yaml — Schema version not bumped after substantial additions (structural + coverage)
    Two new classes (SingleCellLevel1Attributes, AnnDataComplianceMixin) and six new enums were added to a shared base module consumed by multiple assays. If the schema carries a version: field in its header, incrementing it would help downstream consumers detect the breaking change introduced by the scRNA-seq refactor. No strict action required, but worth noting.

Verdict

REQUEST_CHANGES


Rules defined in CLAUDE.md · To update review rules, edit CLAUDE.md and open a PR to main

Auto-generated Python classes from LinkML schema updates.

**Auto-generated by GitHub Actions workflow**
[skip ci]
@github-actions

This comment was marked as duplicate.

Comment thread modules/scATAC-seq/domains/level_3_4.yaml

@PozhidayevaDarya PozhidayevaDarya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just have one comment; otherwise I think things look good.

Comment thread modules/scATAC-seq/domains/level_1.yaml Outdated
Comment thread modules/scATAC-seq/domains/level_1.yaml Outdated
Comment thread modules/scATAC-seq/domains/level_2.yaml Outdated
Comment thread modules/scATAC-seq/domains/level_2.yaml Outdated

@ykatariy ykatariy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few comments about chaning the requirement status of several attributes. They all align with the RFC, but after some thought, I think they should be updated. I recommend these changes be approved by someone with more ATAC-specifc assay knowledge!

…optional

Per review (ykatariy), relax four Level 1/2 fields from required to optional:
- REVERSE_TRANSCRIPTION_PRIMER, SPIKE_IN (L1): ATAC has no reverse-transcription
  step and no standard spike-in; applicable only to multiome / hybrid protocols
- MEDIAN_GENES_PER_CELL (L2): typically only available for multiome, not regular
  scATAC-seq
- MITOCHONDRIAL_READ_PAIRS (L2): sparsely reported for snATAC submissions

Tests updated to assert these are optional. scATAC suite: 103 passed.
@github-actions
github-actions Bot dismissed their stale review July 24, 2026 14:37

Superseded by updated review on commit 7b0610d

github-actions[bot]
github-actions Bot previously approved these changes Jul 24, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTAN Schema Review — ✅ Approved

Automated review by htan-claude · commit 7b0610d27595a5ff99f17a94660a6a33b214361d

The PR is well-architected and the biological modeling is careful, but there are a handful of issues — one blocking slot completeness gap and several substantive biological warnings — that need attention before merge.

Caution

1 blocking issue must be resolved before merge

Files Changed

  • modules/scATAC-seq/domains/scatac_seq.yaml
  • modules/scATAC-seq/domains/level_1.yaml
  • modules/scATAC-seq/domains/level_2.yaml
  • modules/scATAC-seq/domains/level_3_4.yaml
  • modules/scATAC-seq/tests/test_scatac_seq.py
  • modules/Sequencing/domains/sequencing.yaml
  • modules/Sequencing/tests/test_single_cell_layer.py
  • modules/scRNA-seq/domains/level_1.yaml (refactor)
  • modules/scRNA-seq/domains/level_3_4.yaml (refactor)
  • modules/scRNA-seq/tests/ (updated)
  • Root Makefile, CLAUDE.md (registration/docs)

Checklist Results

Check Result Notes
Inheritance correctness PASS All five chains verified; no skipped levels
Inlining of nested objects PASS Container slots correctly inlined: true; identifier-bearing ranges confirmed
Slot completeness (range, title, description) FAIL Container slots on scATACseqData missing title (blocking)
Enum integrity (alphabetical, descriptions) PASS All 10 new/migrated enums alphabetically ordered and fully described
Generated artifacts N/A Intentionally excluded per repo convention; downstream gen PR expected

Findings

Blocking

  • modules/scATAC-seq/domains/scatac_seq.yaml — Container slots missing title (structural + coverage)
    Slots level1_data, level2_data, and level3_4_data on scATACseqData each have range, inlined: true, and description, but all three are missing the required title field. The repo's slot completeness rule applies uniformly — including to container/scaffolding slots. Add title: "Level 1 Data", title: "Level 2 Data", and title: "Level 3/4 Data" respectively to resolve.

Warnings

  • modules/Sequencing/domains/sequencing.yamlLibraryConstructionMethodEnum lacks scATAC-seq-specific library prep methods (biological)
    The enum was migrated from scRNA-seq and its values (Smart-seq, Fluidigm C1, Drop-seq, InDrop) are oriented toward RNA workflows. Since LIBRARY_CONSTRUCTION_METHOD is now a required slot on SingleCellLevel1Attributes — and therefore on scATACLevel1 — scATAC-seq submitters would be forced to select "Other" for essentially all standard Tn5-based workflows (10x Chromium ATAC, Omni-ATAC, etc.), defeating the purpose of a controlled vocabulary. Extend the enum to include at minimum the common single-cell ATAC library prep methods before this module accepts real submissions.

  • modules/scATAC-seq/domains/level_1.yamlNUCLEIC_ACID_SOURCE permits "RNA", which is biologically invalid for scATAC-seq (biological)
    NucleicAcidSourceEnum includes "RNA" and "Unknown", but scATAC-seq (and snATAC-seq) invariably operates on genomic DNA — "RNA" is not a valid input to a transposition-based chromatin-accessibility assay. Consider adding a slot-level equals_string: "DNA" constraint on NUCLEIC_ACID_SOURCE within scATACLevel1, or a schema rule enforcing this, to prevent systematic metadata errors.

  • modules/scATAC-seq/domains/level_1.yamlREVERSE_TRANSCRIPTION_PRIMER and SPIKE_IN lack ATAC-specific scope guidance (biological)
    The PR correctly acknowledges these slots are biologically inapplicable to standard scATAC-seq, but both slots carry descriptions with no ATAC-specific caveat (REVERSE_TRANSCRIPTION_PRIMER simply reads "Primer used for reverse transcription"). Without at minimum a description-level note that these apply only to multiome/hybrid protocols, submitters have no schema guidance against misuse. Update both slot descriptions to explicitly state they are relevant only in multiome or hybrid RNA+ATAC protocols, and consider a schema rule gating them on a protocol-type discriminator.

  • modules/scATAC-seq/domains/level_3_4.yamlSEURAT_CLUSTERS cardinality mismatch (biological)
    SEURAT_CLUSTERS is defined as multivalued: true, range: string, but Seurat cluster assignment is a per-cell categorical label — a singular value per cell stored in AnnData obs. At the file/record level this slot should represent a cluster resolution or obs column name, which is singular. If the intent is to capture multiple clustering resolutions applied to the dataset, the description must explicitly say so. As written the cardinality is misleading; either remove multivalued: true or substantially expand the description to justify list semantics here.

  • Instance tests will error until the downstream gen PR merges (coverage)
    The TestInstances suite in test_scatac_seq.py imports generated dataclasses from modules/scATAC-seq/src/htan_scatac_seq/datamodel/scatac_seq.py, which is intentionally absent from this PR. The dm fixture will fail to resolve at test runtime on this branch, causing those tests to error or be skipped rather than pass. This is not a schema defect, but CI on this branch should be understood to be incomplete until the downstream make modules-gen PR lands. Ensure the PR description or CI notes make this dependency explicit.

Informational

  • modules/scATAC-seq/domains/level_2.yaml — WES-centric QC metric descriptions carried into scATAC Level 2 (biological)
    Slots such as PROPORTION_COVERAGE_10X, PROPORTION_COVERAGE_30X, MEAN_COVERAGE, and PROPORTION_TARGETS_NO_MATCH carry descriptions referencing "whole genome sequencing", "whole exome and targeted sequencing", and "Picard Tools target coverage". These are optional and functional, but the WES-oriented language is likely to confuse scATAC-seq submitters. Worth updating descriptions in a follow-up to clarify applicability (e.g., coverage over peaks or accessible regions) or to note that these are inherited general sequencing QC fields.

  • modules/scATAC-seq/domains/level_3_4.yamlPEAKS_CALLING_SOFTWARE is free-text with no controlled vocabulary (biological)
    Peak-calling software (MACS2, MACS3, Genrich, Homer, etc.) is a meaningful methodological provenance field, but PEAKS_CALLING_SOFTWARE is defined as range: string with no enum backing. ATAC_GENE_ACTIVITY_WORKFLOW_TYPE correctly uses ATACGeneActivityWorkflowTypeEnum; parity would be desirable here. A PeaksCallingSoftwareEnum in a follow-up PR would improve interoperability. No action required now.

  • modules/Sequencing/domains/sequencing.yaml — Schema version not bumped (biological + coverage)
    Two new classes (SingleCellLevel1Attributes, AnnDataComplianceMixin) and six new enums were added to sequencing.yaml, yet the schema version: header does not appear to have been incremented. This is informational — the repo convention on version bumping for substantial structural additions would suggest a bump is appropriate, but it does not block merge.

Verdict

REQUEST_CHANGES


Rules defined in CLAUDE.md · To update review rules, edit CLAUDE.md and open a PR to main

Auto-generated Python classes from LinkML schema updates.

**Auto-generated by GitHub Actions workflow**
[skip ci]
@github-actions

Copy link
Copy Markdown
Contributor

Python classes auto-updated!

The Python classes have been automatically regenerated and committed to this PR branch.

**Updated files:**
```
modules/Biospecimen/src/htan_biospecimen/datamodel/biospecimen.py

modules/Clinical/src/htan_clinical/datamodel/clinical.py
modules/DigitalPathology/src/htan_digitalpathology/datamodel/digital_pathology.py
modules/Imaging/src/htan_imaging/datamodel/imaging.py
modules/MassSpectrometryImaging/src/htan_massspectrometryimaging/datamodel/mass_spectrometry_imaging.py
modules/MultiplexMicroscopy/src/htan_multiplexmicroscopy/datamodel/multiplex_microscopy.py
modules/Sequencing/src/htan_sequencing/datamodel/sequencing.py
modules/SpatialOmics/src/htan_spatial/datamodel/spatial.py
modules/WES/src/htan_wes/datamodel/wes.py
modules/scATAC-seq/src/htan_scatac_seq/datamodel/scatac_seq.py
modules/scRNA-seq/src/htan_scrna_seq/datamodel/scrna_seq.py
```

The generated classes are now up to date with the schema changes.

…riptions

- scatac_seq.yaml: add title to level1_data/level2_data/level3_4_data container
  slots (slot-completeness rule applies to container slots too; matches MSI)
- level_1.yaml: note in REVERSE_TRANSCRIPTION_PRIMER and SPIKE_IN descriptions
  that they apply only to multiome / hybrid RNA+ATAC protocols and should be
  left blank for standard scATAC-seq
@github-actions
github-actions Bot dismissed their stale review July 24, 2026 14:50

Superseded by updated review on commit 8145b9c

github-actions[bot]

This comment was marked as outdated.

Auto-generated Python classes from LinkML schema updates.

**Auto-generated by GitHub Actions workflow**
[skip ci]
@github-actions

This comment was marked as outdated.

Use pytest.importorskip in the dm fixture so the valid/invalid instance tests
skip (not error) on a fresh checkout before `make modules-gen`. The import is
fixture-scoped, so the schema-level tests run regardless.
@github-actions
github-actions Bot dismissed their stale review July 24, 2026 15:05

Superseded by updated review on commit 07a3e4b

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTAN Schema Review — ✅ Approved

Automated review by htan-claude · commit 07a3e4b32032036d6d1c098e97875f908acfcbb2

Solid PR overall — the inheritance design is clean and the test suite is comprehensive, but there are a few biological and coverage issues that need attention before merge.

Caution

2 blocking issues must be resolved before merge

Files Changed

  • modules/scATAC-seq/domains/level_1.yaml
  • modules/scATAC-seq/domains/level_2.yaml
  • modules/scATAC-seq/domains/level_3_4.yaml
  • modules/scATAC-seq/domains/scatac_seq.yaml
  • modules/scATAC-seq/tests/test_scatac_seq.py
  • modules/Sequencing/domains/sequencing.yaml
  • modules/Sequencing/tests/test_single_cell_layer.py
  • modules/scRNA-seq/ (refactored classes and tests)

Checklist Results

Check Result Notes
Inheritance correctness PASS All five chains verified intact; scRNALevel1 refactor preserves the spine
Inlining of nested objects PASS All three container slots carry inlined: true; ranges have identifier: true
Slot completeness (range, title, description) FAIL TOTAL_READS/MAP_Q_30 misplaced at L1; CONTAMINATION_ERROR missing minimum_value
Enum integrity (alphabetical, descriptions) FAIL SingleNucleusBufferEnum missing Other/Unknown; TranspositionReactionEnum has overlapping generic "Tn5" value
Generated artifacts PASS Intentionally excluded per repo convention; no action needed

Findings

Blocking

  • level_1.yamlTOTAL_READS and MAP_Q_30 are alignment-derived metrics placed at Level 1 (biological)
    Both slots are sourced from samtools and can only be computed after read alignment against a reference genome — they require a BAM/CRAM, which is a Level 2 artifact. Marking them required: true on scATACLevel1 (raw FASTQ files) is biologically inconsistent: samtools cannot report these values from a FASTQ. Move both slots to scATACLevel2, where the rest of the samtools-derived metrics (TOTAL_UNIQUELY_MAPPED, LOW_MAP_Q, etc.) already live.

  • level_1.yamlSingleNucleusBufferEnum missing catch-all escape values (biological)
    Every other shared single-cell enum introduced in this PR (DissociationMethodEnum, LibraryConstructionMethodEnum, SingleCellIsolationMethodEnum, etc.) includes both "Other" and "Unknown". SingleNucleusBufferEnum exposes only four named buffers ("10x", "NIB", "Omni", "TST") with no escape hatch. Reagent kits evolve rapidly, and submissions using novel buffer formulations will fail schema validation without an "Other" option. Add at minimum "Other" and "Unknown" to align with the repo-wide pattern.

Warnings

  • level_1.yamlNUCLEUS_IDENTIFIER conflates per-read barcode identity with file-level metadata (biological + coverage)
    The slot is described as "Unique nuclei barcode; added at transposition step. Determines which nucleus the reads originated from." This is a per-read/per-molecule attribute captured in the FASTQ read header or barcode whitelist — not a scalar string value that can be recorded once per file across a multi-nucleus experiment. Additionally, the coverage reviewer notes the slot reads semantically like an identifier but lacks identifier: true and a corresponding pattern: regex. Consider renaming to NUCLEI_BARCODE_WHITELIST_PATH or NUCLEI_BARCODE_SCHEME with a clarified description, or confirm with the team whether NUCLEI_BARCODE (already present and optional) covers the same intent.

  • level_1.yamlTranspositionReactionEnum has an ambiguous generic "Tn5" value that overlaps named variants (biological)
    "Tn5" as a permissible value is a generic enzyme name that subsumes "Tn5-059", "Nextera Tn5", and arguably "EZ-Tn5". A submitter using standard Tn5 transposase could validly select either "Tn5" or a more specific variant, creating enum exclusivity ambiguity. Either remove the generic "Tn5" entry (if all real-world protocols map to a named variant) or rewrite its description to explicitly scope it as a catch-all for Tn5-based protocols not covered by the named entries. Also consider adding "Other" for non-Tn5 chemistries not present in the list.

  • scatac_seq.yaml — Container slots missing explicit required: annotation (structural)
    level1_data, level2_data, and level3_4_data all carry range and inlined: true but no required: field. Per slot-completeness rules, required/optional intent must be explicit. If these slots are always mandatory (a container with no level data is meaningless), mark them required: true; if a record may carry only one level at a time, mark the others required: false. The omission leaves intent ambiguous for future maintainers and automated validators.

  • level_2.yamlCONTAMINATION_ERROR missing minimum_value: 0.0 (biological + coverage)
    Every other float QC metric in scATACLevel2 carries minimum_value: 0.0; CONTAMINATION_ERROR (a standard-error estimate from GATK4) is also a non-negative quantity but lacks this bound. The omission is inconsistent with the rest of the file and allows negative values to pass schema validation. Add minimum_value: 0.0.

  • test_scatac_seq.py — Eight optional Level 3/4 slots have no not-required assertion in tests (coverage)
    DNASE_SENSITIVE_REGION_FRAGMENTS, ENHANCER_REGION_FRAGMENTS, PROMOTER_REGION_FRAGMENTS, ON_TARGET_FRAGMENTS, BLACKLIST_REGION_FRAGMENTS, PEAK_REGION_FRAGMENTS, PEAK_REGION_CUTSITES, and NUCLEOSOME_PERCENTILE are declared optional in the schema but are absent from LEVEL_OPTIONAL["scATACLevel3and4"] in the test file. The coverage rules require optional slots to be asserted as not-required in tests. Add these eight slots to the LEVEL_OPTIONAL dict to close the gap.

  • test_scatac_seq.py — Invalid-instance coverage depends entirely on generated dataclasses (coverage)
    The test_missing_required_raises and test_bad_enum_value_raises tests rely on generated Python dataclasses enforced via pytest.importorskip, meaning on a fresh checkout (before make modules-gen) these tests are silently skipped. The coverage rules require at least one invalid-instance test that unconditionally raises. Consider adding a schema-level invalid-instance check using linkml_runtime or linkml.validators as a fallback so the check runs regardless of whether generated artifacts are present.

Informational

  • level_1.yamlREVERSE_TRANSCRIPTION_PRIMER and SPIKE_IN on a DNA-accessibility assay (biological)
    The PR description acknowledges these slots are biologically inapplicable to standard scATAC-seq and flags them for community review. Both are required: false and the rationale (multiome/hybrid protocols) is documented in slot descriptions. Worth noting that SpikeInEnum values ("ERCC", etc.) are all RNA spike-in standards — if the slots are retained long-term, a "Not applicable" value may be more accurate than "None" for submitters of pure ATAC experiments. No action required at merge time.

  • sequencing.yaml — Schema version not visibly bumped after substantial additions (coverage)
    New classes (SingleCellLevel1Attributes, AnnDataComplianceMixin) and six new enums were added to the Sequencing base module, which is consumed by multiple downstream modules. If this module tracks a version: field, it should be incremented. If the module does not use schema versioning, no action is needed — authors should confirm which applies.

  • level_3_4.yamlSEURAT_CLUSTERS declared multivalued: true on a logically-singular descriptor (biological)
    The slot describes "Clusters of cells by a shared nearest neighbor (SNN) modularity optimization based clustering algorithm." A clustering result is typically a single string (resolution parameter or annotation reference), not multiple independent strings. If the intent is to record multiple cluster label sets (e.g., from different resolutions), the description should clarify what each element represents. As-is the semantics are ambiguous, but this is a design question for the team rather than a schema error.

  • level_3_4.yamlATAC_GENE_ACTIVITY_WORKFLOW_TYPE slot and enum share near-identical descriptions (coverage)
    The slot description ("Generic name for the workflow used to analyze a data set") and ATACGeneActivityWorkflowTypeEnum description are nearly verbatim duplicates. No structural issue; flagged for awareness in case a more precise slot description would help submitters understand the intended scope.

Verdict

REQUEST_CHANGES


Rules defined in CLAUDE.md · To update review rules, edit CLAUDE.md and open a PR to main

Auto-generated Python classes from LinkML schema updates.

**Auto-generated by GitHub Actions workflow**
[skip ci]
@github-actions

Copy link
Copy Markdown
Contributor

Python classes auto-updated!

The Python classes have been automatically regenerated and committed to this PR branch.

**Updated files:**
```
modules/Biospecimen/src/htan_biospecimen/datamodel/biospecimen.py

modules/Clinical/src/htan_clinical/datamodel/clinical.py
modules/DigitalPathology/src/htan_digitalpathology/datamodel/digital_pathology.py
modules/Imaging/src/htan_imaging/datamodel/imaging.py
modules/MassSpectrometryImaging/src/htan_massspectrometryimaging/datamodel/mass_spectrometry_imaging.py
modules/MultiplexMicroscopy/src/htan_multiplexmicroscopy/datamodel/multiplex_microscopy.py
modules/Sequencing/src/htan_sequencing/datamodel/sequencing.py
modules/SpatialOmics/src/htan_spatial/datamodel/spatial.py
modules/WES/src/htan_wes/datamodel/wes.py
modules/scATAC-seq/src/htan_scatac_seq/datamodel/scatac_seq.py
modules/scRNA-seq/src/htan_scrna_seq/datamodel/scrna_seq.py
```

The generated classes are now up to date with the schema changes.

@aditigopalan
aditigopalan merged commit 0e871f4 into main Jul 24, 2026
@aditigopalan
aditigopalan deleted the RFC-scATACseq branch July 24, 2026 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: Implement sc-ATACseq

4 participants