Skip to content

[ENH] Recommend controlled vocabulary for age Units, clarify that it can be overloaded#2400

Merged
yarikoptic merged 5 commits intobids-standard:masterfrom
yarikoptic:clarify-age
May 7, 2026
Merged

[ENH] Recommend controlled vocabulary for age Units, clarify that it can be overloaded#2400
yarikoptic merged 5 commits intobids-standard:masterfrom
yarikoptic:clarify-age

Conversation

@yarikoptic
Copy link
Copy Markdown
Collaborator

@yarikoptic yarikoptic commented Apr 16, 2026

Of high relevance to @bids-standard/bep032 effort, but also potentially to @bids-standard/bep036 and others.

TODOs/Notes

  • it will make some bids-example datasets invalid since they use "years" for "Units" instead of "year", submitted 'Units' value must be singular, not plural (year not years) bids-examples#553
  • Review/fix for that test fix. Unclear why NOW it became applicable/failing
    • we ran into this since this is presumably the first rule adding custom checks on the column definition (not sidecar) .json files? (according to claude). But reworked test selection seems became too cumbersome
    • no more changes to tests -- validating against .tsv
  • For BIDS 2.0 make 'warning' into 'error' there Units not being from allowed values. Added to TODOs of BIDS-2.0 #1775

@yarikoptic yarikoptic requested a review from effigies April 16, 2026 01:02
@yarikoptic yarikoptic added schema Issues related to the YAML schema representation of the specification. Patch version release. documentation labels Apr 16, 2026
yarikoptic added a commit to yarikoptic/BIDS-examples that referenced this pull request Apr 16, 2026
… years)

Overall specification and testing is proposed for this in

bids-standard/bids-specification#2400

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "bash -c 'sed -i -E '\"'\"'s,(Units.: .year)s,\\1,g'\"'\"' {outputs}'",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [
  "*/participants.json"
 ],
 "pwd": "."
}
^^^ Do not change lines above ^^^
Copy link
Copy Markdown
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Some comments. Overall, I think this is a good push and will help clarity, but needs to be done in a backwards compatible way.

Comment thread src/schema/rules/checks/dataset.yaml Outdated
Comment thread src/schema/rules/checks/dataset.yaml Outdated
Comment thread src/schema/objects/columns.yaml Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.07%. Comparing base (6764039) to head (5c6c320).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2400   +/-   ##
=======================================
  Coverage   83.07%   83.07%           
=======================================
  Files          22       22           
  Lines        1696     1696           
=======================================
  Hits         1409     1409           
  Misses        287      287           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

yarikoptic and others added 2 commits April 16, 2026 11:12
Document in common-principles.md that age Units MAY be overridden
to one of: year, month, week, day, hour, minute, or second (based on
ISO 8601 duration designators).  Add AgeUnits schema check rule that
validates participants.json age Units against the allowed set.

Co-Authored-By: Claude Code 2.1.110 / Claude Opus 4.6 <noreply@anthropic.com>
@yarikoptic
Copy link
Copy Markdown
Collaborator Author

@effigies I forced pushed rebase on master with tune up the the testing... if you see how to improve it please share (even if an idea). according to claude this is "AgeUnits rule is the first to access json.age in participants.json" and thus we started to hit in this test for sidecar json files.

Comment thread tools/schemacode/src/bidsschematools/tests/test_expressions.py Outdated
Comment thread tools/schemacode/src/bidsschematools/tests/test_expressions.py Outdated
Comment thread tools/schemacode/src/bidsschematools/tests/test_expressions.py Outdated
Comment thread src/schema/rules/checks/dataset.yaml Outdated
yarikoptic and others added 3 commits April 16, 2026 12:28
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@yarikoptic
Copy link
Copy Markdown
Collaborator Author

done, rewritten, force pushed, I think we have converged!

@yarikoptic yarikoptic mentioned this pull request Apr 16, 2026
13 tasks
@effigies
Copy link
Copy Markdown
Collaborator

Just for the sake of considering alternatives, the ISO 8601 standard for durations is P47Y1M8D and such-like. We could allow someone to use "Format": "ISO8601-Duration" and define the regex /P(\d+Y)?(\d+M)?.../ to validate those durations.

That would be using an existing standard instead of creating our own controlled vocabulary inspired by the standard, so I think it's worth considering, if only to record a reason for rejecting it.

My reasons would be:

  1. Existing practice is to use decimal durations, and providing a common set of unit names is more practical than suggesting everybody learn to read and write ISO strings.
  2. Including it as an optional unit (e.g., "Units": "ISO8601") would be a burden on dataset consumers.

@effigies effigies changed the title [SCHEMA] Clarify age Units, validate permissible value for "age" and how to overload [ENH] Recommend controlled vocabulary for age Units, clarify that it can be overloaded Apr 16, 2026
@yarikoptic
Copy link
Copy Markdown
Collaborator Author

yeap, search reminded me mentioning it in original

FWIW, I think adopting standard for durations makes sense. I will extract your comment into an issue

@yarikoptic
Copy link
Copy Markdown
Collaborator Author

@ericearl I would like to invite you to review this PR since somewhat relates to @bids-standard/bep036 and thus could be of interest for feedback or just approval ;) @julia-pfarr also might be of interest to you as a liaison to the world of all the participants.tsv annotations of neurobagels ;)

Comment thread src/schema/rules/checks/dataset.yaml
Copy link
Copy Markdown
Collaborator

@ericearl ericearl left a comment

Choose a reason for hiding this comment

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

Find a resolution to my one comment, and it looks good!

@effigies
Copy link
Copy Markdown
Collaborator

Two approvals. This can be merged on/after Thursday, April 23, to ensure there's enough time for people to review since the last significant edit.

@julia-pfarr
Copy link
Copy Markdown
Member

Looks good! FYI: Neurobagel uses age in years with different formats (float (31.5), range (30-35), euro (31,5), bounded (30+), iso8601 (31Y6M))

@yarikoptic
Copy link
Copy Markdown
Collaborator Author

Two approvals. This can be merged on/after Thursday, April 23, to ensure there's enough time for people to review since the last significant edit.

coolio, today is May 07 so let's proceed!

@yarikoptic yarikoptic merged commit 9d488b7 into bids-standard:master May 7, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bids-2.0 documentation schema Issues related to the YAML schema representation of the specification. Patch version release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Formalize participants' age to actually allow for other than year units/durations

4 participants