You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add the BER feature model as an imported module - #5
This adds the BER feature model (contigs, genome features, their locations and attributes) to the central schema as an imported module, with valid example data. The dialect schemas, conversion profiles, converters and validator stay in https://github.com/turbomam/feature-table-corpus and are deferred; this branch carries none of them (turbomam/feature-table-corpus#131). It follows the pattern of the measurements module from #2. Draft for discussion at the GenomeFeatures review on 2026-10-02.
ber_central_schema.yaml imports ber_feature_model beside measurements, and also declares the MIXS and EDAM_DATA prefixes. Without them the generated Python fails with NameError, because it doesn't define prefixes that only an imported module declares when enum meanings use them.
The copy keeps only the schema. Notes and descriptions about the conversion tooling, and references to files in feature-table-corpus, are removed; one note points to the v0.1.0 release for the evidence behind each element. tree_root is dropped from Dataset so this module does not set the root class for the whole central schema.
Two edits to the copy, both needed to build here:
The name slot is renamed display_name with alias: name, because the root schema also defines name and gen-project refuses two imported slots with one name ("Conflicting URIs"). Data still uses the key name.
.linkmllint.yaml marks StrandEnum's GFF3 symbols (+, -, ., ?) as deliberate for just lint. Lint still reports EDAM_DATA twice, because it recommends the dotted EDAM.DATA. Its other two warnings are in measurements.yaml and are not changed here.
Example data
All public, all converted with the model's own tools:
tests/data/valid/Dataset-002.yaml: an NMDC run's contigs, its contig collection and 15 features. Dataset-003.yaml: phiX174 (NC_001422.1) converted with insdc-locations/1.0.0, every feature with a structured location, some crossing the origin. Dataset-004.yaml: the Pfam hits of Dataset-001 converted with nmdc-pfam-protein/5.0.0, each with its alignment target.
tests/data/invalid/ has one example per added class, each breaking one constraint named in its filename: Contig-zero-length, ContigCollection-unknown-type, FeatureLocation-unknown-operator, LocationPart-zero-start, AlignmentTarget-missing-target-id, Attribute-missing-value, and Dataset-undeclared-slot.
just test passes locally: 7 data tests and the examples run. linkml-validate against the root schema accepts all four valid Datasets and rejects each invalid example for exactly the fault its name gives (checked 2026-10-06).
Open questions
"Enumerate them": should the model reuse linkml/valuesets enums (ContigCollectionType has the same seven values, uppercased (ISOLATE, MAG, ...), so reusing it means mapping our lowercase values; StrandType and CdsPhaseType cover the same meanings with different spellings), or does this mean listing the model's elements?
Is renaming name to display_name with an alias acceptable, or should the root's name and the model's be reconciled into one slot?
KBase's CDM (https://github.com/kbase/cdm-schema, src/linkml/cdm_bioentity.yaml) already defines Contig, ContigCollection, Feature, StrandType, CdsPhaseType and ContigCollectionType. Should this module reuse those classes, map to them, or stay separate? ContigCollection already notes the mapping.
Copied from turbomam/feature-table-corpus at 4aaca94 (ber_feature_model.yaml 0.1.0 and
attributes.yaml), following the measurements module's pattern. Two changes from the copy, both
needed for this repository's build: the name slot becomes display_name with alias name, since
the root schema defines name; and the EDAM.DATA prefix becomes EDAM, which gen-python can emit.
MIXS and EDAM are also declared in the root so the generated Python defines them.
Remove notes, comments and description wording that describe the
feature-table-corpus conversion tooling or cite files in that repository,
which do not exist here. Keep short notes that define the model itself,
and point to the v0.1.0 release for the evidence behind each element.
Drop tree_root from Dataset so the imported module does not set the root
class for the whole central schema.
…FF symbols as deliberate in lint
feature-table-corpus spells the prefix EDAM_DATA since turbomam/feature-table-corpus#163; the IRIs
are unchanged. .linkmllint.yaml excludes the four GFF3 strand symbols from standard_naming.
Lint still reports EDAM_DATA, because the canonical EDAM.DATA can't be emitted by gen-python
(linkml/linkml#3458).
The description promises that repeated Note values remain in source order, while this multivalued slot is unordered in the LinkML model. Add list_elements_ordered so generated representations can preserve the documented ordering.
This issue also appears in the following locations of the same file:
The display_name/name alias is an integration-specific compatibility behavior, but none of the Dataset examples contains a name field, so the automated runtime/example tests do not protect the behavior verified manually in the review thread. Add name to a valid Feature or ContigCollection example to make this a regression test.
Answering the two previously-missed items in Copilot's review of bfd6712 (#5 (review)):
Alias coverage: fixed in 3422d4d. The first Pfam hit in tests/data/valid/Dataset-001.yaml now has name: DDE_3, from its own Name attribute, so just test loads a name key through alias: name. linkml-validate accepts that file. The same file with display_name: instead is rejected as an unexpected property.
Ordered lists: right about note, dbxref and ontology_term ("in source order") and taxonomic_lineage (rank order). The source model has the same gap, so it's filed there to fix first and then copy here: Mark the four order-dependent list slots as ordered turbomam/feature-table-corpus#170 (Mark the four order-dependent list slots as ordered). Changing only this copy would make it drift from feature-table-corpus.
The valid examples are real data converted by the model's own profiles: an NMDC run with its
contig collection, phiX174 with joined and origin-crossing locations, and Pfam hits with their
alignment targets. Each invalid example breaks one constraint and is named for it.
This copy still advertises the upstream schema's id and 0.1.0 version, but it is structurally different from that release: upstream v0.1.0 has Dataset.tree_root, the name slot, and EDAM.DATA, while this artifact drops or renames them. Two non-equivalent contracts are therefore indistinguishable by (id, version), which can misidentify generated artifacts and cached schemas. Publish this as a distinct derived version, or release these changes upstream and vendor that exact version.
Answering the previously-missed item in Copilot's review of b632020 (#5 (review)): right, the copy differed from v0.1.0 while advertising the same id and version. Fixed in 15b0e06. The copy's version is now 0.1.0-bridge.1, and a note lists each difference from v0.1.0: no tree_root, display_name with alias name, the EDAM_DATA prefix, and the ordered lists and '.' wording from turbomam/feature-table-corpus#172, which merged after v0.1.0 and is copied here too. The id stays the same, so the copy can align with a later feature-table-corpus release.
Thanks for this work, @turbomam. Could we consider using something like SequenceFeature instead of straight up Feature? Feature is also used in the earth science world with the base Feature concept being quite broad and then qualified for specific types of features, e.g., SamplingFeature (ref: OGC Observations and Measurements) = point in a stream, soil pit, tree, etc.
It is totally overwhelming to have so much back and forth with the agent and to have reams of LLM text to wade through as part of the PR -- can we institute an SOP that keeps the LLM iteration out of the PRs or reserved for draft PRs, which are then cleaned up for human consumption?
I usually skim through the conversation tab first so I can see what others have already brought up, and pages of back and forth with the agent make it hard to pick out what, if anything, is of value. It seems like this iterative co-development should be done prior to making the PR, rather than as part of it. Can we reserve PRs for finished products, ready for human comments, not WIPs? Or, if they are WIPs, label them as being agent co-dev WIPs that other humans do not need to look at?
It is totally overwhelming to have so much back and forth with the agent
Thanks @ialarmedalien. Yes, I like the idea of a SOP and appreciate your feedback. I use Claude as my main development agent and then like to have have copilot or codex review give them an independent review. That does make the PRs long but it keeps the evolution of the PR transparent. As long as I have an Open AI allocation though the Rosalind program I can do the orthogonal reviews locally and not post it's findings. I think @cmungall and other BBOP people have other review patterns and I'm not sure how verbose they are but will look into it.
I can also rewrite the PR description by hand and hide some of the agentic replies that came after that.
I don't know how valuable it is to keep iterations of the PR content visible. When I look at a PR, I don't really care how it got to its current state; I evaluate the code/text/whatever it is as presented on screen. Certainly for work that I do, I can't imagine anyone's going to want to see how the code evolved to its present state, and it's not really relevant as the older versions aren't going into the repo.
If there is something surprising in a PR (an unintuitive choice, something that appears illogical, etc.), comments in the code or inline in the PR are appropriate and reviewers can always ask Qs.
The reason will be displayed to describe this comment to others. Learn more.
I support AJ's request for attribute/value to be moved into a separate PR to be reviewed/merged prior to this PR.
Nothing in particular jumps out at me as being problematic in this schema, but I should solicit some sample data from IMG, SMC, Phytozome, and Mycocosm in order to provide enough test coverage for you.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
valerie-autumn-skye
left a comment
Why
This adds the BER feature model (contigs, genome features, their locations and attributes) to the central schema as an imported module, with valid example data. The dialect schemas, conversion profiles, converters and validator stay in https://github.com/turbomam/feature-table-corpus and are deferred; this branch carries none of them (turbomam/feature-table-corpus#131). It follows the pattern of the measurements module from #2. Draft for discussion at the GenomeFeatures review on 2026-10-02.
What changed
ber_feature_model.yamlandattributes.yaml, copied from the feature-table-corpus v0.1.0 release (https://github.com/turbomam/feature-table-corpus/releases/tag/v0.1.0, commit 4aaca94). They keep their own id, prefixes and CC0 1.0 license. Because the copy differs from v0.1.0 in the ways listed below, its version is 0.1.0-bridge.1. It also includes Mark the four order-dependent list slots as ordered, and fix the '.' strand description turbomam/feature-table-corpus#172 (ordered lists, '.' strand wording), which merged after v0.1.0.ber_central_schema.yamlimportsber_feature_modelbesidemeasurements, and also declares theMIXSandEDAM_DATAprefixes. Without them the generated Python fails withNameError, because it doesn't define prefixes that only an imported module declares when enum meanings use them.tree_rootis dropped fromDatasetso this module does not set the root class for the whole central schema.nameslot is renameddisplay_namewithalias: name, because the root schema also definesnameandgen-projectrefuses two imported slots with one name ("Conflicting URIs"). Data still uses the keyname.EDAM.DATAprefix becomesEDAM_DATA, with meanings such asEDAM_DATA:1667. These expand to the same IRIs;gen-pythoncan't emit a prefix with a dot in it (pythongen: Unsafe prefix names produce invalid or broken Python — no validation against keywords, builtins, or non-identifier characters linkml/linkml#3458). feature-table-corpus uses the same spelling since Rename EDAM.DATA prefix and test building the BER feature model under an importer turbomam/feature-table-corpus#163 (Make the feature model build when another schema imports it)..linkmllint.yamlmarks StrandEnum's GFF3 symbols (+,-,.,?) as deliberate forjust lint. Lint still reportsEDAM_DATAtwice, because it recommends the dottedEDAM.DATA. Its other two warnings are inmeasurements.yamland are not changed here.Example data
All public, all converted with the model's own tools:
tests/data/valid/Dataset-001.yaml: one CDS and its three Pfam hits from NMDC annotation runnmdc:wfmgan-11-5xxrm214.2(CC BY 4.0), from https://github.com/turbomam/feature-table-corpus/blob/v0.1.0/model/examples/multiple-pfams/harmonized.yamltests/data/valid/Dataset-002.yaml: an NMDC run's contigs, its contig collection and 15 features.Dataset-003.yaml: phiX174 (NC_001422.1) converted withinsdc-locations/1.0.0, every feature with a structured location, some crossing the origin.Dataset-004.yaml: the Pfam hits of Dataset-001 converted withnmdc-pfam-protein/5.0.0, each with its alignment target.tests/data/invalid/has one example per added class, each breaking one constraint named in its filename:Contig-zero-length,ContigCollection-unknown-type,FeatureLocation-unknown-operator,LocationPart-zero-start,AlignmentTarget-missing-target-id,Attribute-missing-value, andDataset-undeclared-slot.NC_003888.3: https://github.com/turbomam/feature-table-corpus/blob/v0.1.0/corpus/derived-examples/actinorhodin.gff3 , converted withgff3-contig/4.0.0. The converted Dataset (22 genes, 22 CDS) also validates against this branch's root schema.just lakehouse-exportin feature-table-corpus (https://github.com/turbomam/feature-table-corpus/blob/v0.1.0/docs/lakehouse-export.md) andfeature-convertin nmdc-lakehouse write the model as Parquet. feature-table-corpus commits the actinorhodin export as Parquet (https://github.com/turbomam/feature-table-corpus/tree/main/model/examples/actinorhodin, added after v0.1.0); none is in BERDL yet (Upload BER feature-model tables to BERDL microbiomedata/nmdc-lakehouse#388).Validation
just testpasses locally: 7 data tests and the examples run.linkml-validateagainst the root schema accepts all four valid Datasets and rejects each invalid example for exactly the fault its name gives (checked 2026-10-06).Open questions
ContigCollectionTypehas the same seven values, uppercased (ISOLATE,MAG, ...), so reusing it means mapping our lowercase values;StrandTypeandCdsPhaseTypecover the same meanings with different spellings), or does this mean listing the model's elements?nametodisplay_namewith an alias acceptable, or should the root'snameand the model's be reconciled into one slot?src/linkml/cdm_bioentity.yaml) already definesContig,ContigCollection,Feature,StrandType,CdsPhaseTypeandContigCollectionType. Should this module reuse those classes, map to them, or stay separate?ContigCollectionalready notes the mapping.