[ENH] Allow plus signs in labels #1926

tsalo · 2024-09-20T13:13:18Z

Closes #1165. Adds + to the valid characters for the "label" format, which is currently only used for a subset of entities.

tsalo · 2024-09-20T13:21:29Z

In this PR, plus signs are allowed for any label-format value, but I believe some folks (@effigies?) were thinking of limiting them to special multi-label cases. If we want to go down that route, then we'll need to create a new "format" (e.g., "multilabel") that applies to specific entities. I'm not 100% which entities we would want the multilabel format for... perhaps acq, desc, and space?

effigies · 2024-09-20T14:00:28Z

Reading #1165 (comment) and the following discussion, I think the overall consensus was toward permissiveness; i.e., allow in any label instead of splitting out a new multi-label concept.

I feel like the discussion about "semantics" was not entirely clear. I personally feel we should refrain from implying a relation between ent-X+Y and ent-X or ent-Y that tooling would need to respect. Perhaps we want to say something like:

Free-form labels with alphanumeric and plus (+) characters.
Plus signs MAY be used to concatenate multiple applicable labels,
but no relationship is established by a partial match.
In particular, the inheritance principle does not connect files
containing ent-X+Y and ent-X or ent-Y.

tsalo · 2024-09-20T14:02:47Z

I like that!

oesteban

Looks good, just a couple of nit picks

CODEOWNERS

src/schema/objects/formats.yaml

Co-authored-by: Chris Markiewicz <[email protected]>

…ts to gain + === Do not change lines below === { "chain": [], "cmd": "git-sedi '\\[0-9a-zA-Z\\]' '[0-9a-zA-Z+]'", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

…+ as well

yarikoptic · 2024-09-23T15:42:05Z

tools/schemacode/bidsschematools/tests/test_rules.py

-            r"(?:sub-(?P<subject>[0-9a-zA-Z]+)/)?"
-            r"(?:ses-(?P<session>[0-9a-zA-Z]+)/)?"
+            r"(?:sub-(?P<subject>[0-9a-zA-Z+]+)/)?"
+            r"(?:ses-(?P<session>[0-9a-zA-Z+]+)/)?"


I am afraid that is not all ... I will push a few commits on that end in a few minutes (I hope you don't mind).
Some other might need adjustment and I even start feeling that we might need to come up with some term (like "literal" but there might be better) to encompass "alphanumeric" and +.

@yarikoptic Note this is a regression test that shows the specific output of a specific synthetic rule.

my comment is not really about this test -- I meant that changes in this PR (just this test) aren't sufficient. pushed now

yarikoptic · 2024-09-23T15:42:29Z

Just a sidenote FTR: There is also a question of either to allow it in suffixes (e.g. would converge DANDI layout closer (dandi/dandi-cli#1498) but since we do not have any "incident" of that in BIDS ATM, it is not appropriate to change ATM even though in general I see that suffixes also should be of the same nature as "labels", in particular that IIRC their values could "migrate" into _mod- entity.

yarikoptic · 2024-09-23T15:53:54Z

I pushed some changes which I think are due as well, although might need tune ups -- individual commits might have more information/reasoning in the commit messages. But there is also IMHO outstanding review and possibly tune up needed to src/metaschema.json

it does have plenty of "alphanumeric" and _ allowances and I am not yet 100% sure none of them somehow overlaps with "alphanumeric" possible in filename etc... most likely none, but still worth looking at IMHO

❯ git grep 'a-zA-Z0-9[^+]' -- src/metaschema.json
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:            "^[a-zA-Z0-9][a-zA-Z0-9_-]*$": {
src/metaschema.json:            "^_[a-zA-Z0-9_-]+$": {
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:                "^[a-zA-Z0-9_]+$": {
src/metaschema.json:                    "^[a-zA-Z0-9_]+$": {
src/metaschema.json:                    "^[a-zA-Z0-9_]+$": {
src/metaschema.json:                    "^[a-zA-Z0-9_]+$": { "$ref": "#/definitions/suffixRule" }
src/metaschema.json:                    "^[a-zA-Z0-9_]+$": { "$ref": "#/definitions/suffixRule" }
src/metaschema.json:            "^[a-zA-Z0-9_]+$": {
src/metaschema.json:        "^[a-zA-Z0-9_]+$": {
src/metaschema.json:                "^[a-zA-Z0-9_]+$": {
src/metaschema.json:        "^[a-zA-Z0-9_]+$": {
src/metaschema.json:                "^[a-zA-Z0-9_]+$": {
src/metaschema.json:          "items": { "type": "string", "pattern": "^[a-zA-Z0-9]+$" }

tools/schemacode/bidsschematools/tests/test_validator.py

Remi-Gau

For something like this I would much prefer that one bids example is modified to showcase this so that downstream validator and parsers have got some food to sink their digital teeth into.

tsalo · 2024-09-28T12:41:00Z

@Remi-Gau that's a great point. Do you think it makes more sense to create a new example dataset or to modify an existing one?

I took a quick look at the BIDS examples and https://github.com/bids-standard/bids-examples/tree/master/ds004332 looks promising. There are complex acq entities that seem to convey multiple discrete pieces of information. For example, acq-mpragePMCoff, acq-mpragePMCon, acq-t2starPMCoff, and acq-t2starPMCon could be changed to acq-mprage+pmc+off etc.

EDIT: If you think creating a new dataset makes more sense, then I think it would be good to use something like CUBIDS since that will use the acq entity to flag different kinds of variants (e.g., different numbers of volumes, different flip angles) in a dataset.

Remi-Gau · 2024-09-28T12:51:26Z

I am afraid that if we start adding a new dataset for every new aspect of the spec we need to validate we will end up with too many examples.

How about adding this to the synthetic example?

tsalo · 2024-09-28T12:56:16Z

That works! I'll look into modifying https://github.com/bids-standard/bids-examples/tree/master/synthetic.

Co-authored-by: Yaroslav Halchenko <[email protected]>

codecov · 2025-01-23T17:23:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.45%. Comparing base (f0e14a2) to head (abce472).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1926   +/-   ##
=======================================
  Coverage   82.45%   82.45%           
=======================================
  Files          17       17           
  Lines        1499     1499           
=======================================
  Hits         1236     1236           
  Misses        263      263

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

yarikoptic

FWIW, looks good and complete to me. Should we proceed with the merge?

tsalo · 2025-02-05T21:28:57Z

I never got the bids example updated unfortunately. Maybe I could make time late next week to do it, but if someone else is willing to do it that would be great.

Ref: bids-standard/bids-specification#1926

yarikoptic · 2025-02-05T22:48:59Z

quick&dirty:

Improve synthetic with + in task label for beh/ data bids-examples#481

tsalo · 2025-02-10T17:37:16Z

@yarikoptic thanks for opening the examples PR! I think it's ready to merge now.

tsalo · 2025-02-24T14:30:51Z

@Remi-Gau could you check this over one last time? I think you're the only reviewer who hasn't approved.

tsalo · 2025-02-24T17:41:40Z

Thanks @Remi-Gau! Alright, does anyone have any issue with me merging? Otherwise, I'll merge tomorrow morning.

* origin/master: (76 commits) [pre-commit.ci] pre-commit autoupdate add Julia Pfarr as current BIDS maintainer [pre-commit.ci] pre-commit autoupdate [pre-commit.ci] pre-commit autoupdate fix fix [MAINT] deal with links redirects Temporarily disable dicomlookup urls check Fix URL to renamed now "Contributors" wiki doc: correct typos doc: remove mentioning of `schemacode` install as the third command enh: use macro for the physio template enh: add metaentities to rules README Apply suggestions from code review [ENH] Allow plus signs in labels (#1926) [pre-commit.ci] pre-commit autoupdate [MAINT] update link to new website sty: pyupgrade --py39-plus chore: Bump schema package to 1.0.4-dev chore: Bump schema package to 1.0.3 ... Conflicts: src/schema/objects/columns.yaml -- added the 3 pipette columns

Ref: bids-standard/bids-specification#1926

Allow plus signs in labels.

0afce3c

tsalo added the schema Issues related to the YAML schema representation of the specification. Patch version release. label Sep 20, 2024

tsalo requested a review from erdalkaraca as a code owner September 20, 2024 13:13

Update test_rules.py

e32984c

Remove self from CODEOWNERS.

97de148

tsalo requested review from yarikoptic, effigies and oesteban September 20, 2024 13:38

Update formats.yaml

8400bc6

This was referenced Sep 20, 2024

[ENH] Add volspace entity for CIFTI files #1900

Closed

Remove cohort entity and append cohort to space nipreps/fmriprep#3365

Open

effigies approved these changes Sep 22, 2024

View reviewed changes

oesteban approved these changes Sep 23, 2024

View reviewed changes

CODEOWNERS Show resolved Hide resolved

src/schema/objects/formats.yaml Outdated Show resolved Hide resolved

tsalo and others added 4 commits September 23, 2024 11:05

Merge branch 'master' into plus-sign-labels

aa9c73c

Update src/schema/objects/formats.yaml

a950d08

Co-authored-by: Chris Markiewicz <[email protected]>

Adjust (or replace) mentionings of alphanumeric as now would include …

1481dc6

…+ as well

yarikoptic reviewed Sep 23, 2024

View reviewed changes

Allow for + in the paths, which can include labels

d692608

yarikoptic requested a review from DimitriPapadopoulos as a code owner September 23, 2024 15:50

tsalo commented Sep 23, 2024

View reviewed changes

tools/schemacode/bidsschematools/tests/test_validator.py Outdated Show resolved Hide resolved

Remi-Gau requested changes Sep 28, 2024

View reviewed changes

yarikoptic mentioned this pull request Oct 2, 2024

[ENH] Allow for - (dash) (and/or +) in <label> #1165

Closed

effigies mentioned this pull request Dec 13, 2024

[ENH] revisions to @oesteban's Alternative proposal to BEP038 oesteban/bids-specification#1

Closed

effigies added this to the 1.10.1 milestone Jan 14, 2025

effigies and others added 4 commits January 14, 2025 14:39

Merge remote-tracking branch 'upstream/master' into plus-sign-labels

45643a5

Update src/appendices/entity-table.md

304f0de

Co-authored-by: Yaroslav Halchenko <[email protected]>

fix(test): Run is an index, not a label

e076d05

Merge branch 'master' into plus-sign-labels

abce472

yarikoptic approved these changes Feb 5, 2025

View reviewed changes

yarikoptic added a commit to yarikoptic/BIDS-examples that referenced this pull request Feb 5, 2025

Adjust create_synthethic_ds.sh to use 2 tasknames with +

451d08a

Ref: bids-standard/bids-specification#1926

yarikoptic mentioned this pull request Feb 5, 2025

Improve synthetic with + in task label for beh/ data bids-standard/bids-examples#481

Open

Merge branch 'master' into plus-sign-labels

f551f7d

tsalo mentioned this pull request Feb 12, 2025

Use +s to delimit variant elements PennLINC/CuBIDS#433

Open

tsalo requested a review from Remi-Gau February 13, 2025 15:36

Merge branch 'master' into plus-sign-labels

75924cf

This was referenced Feb 24, 2025

ENH: Allow plus signs in labels bids-standard/pybids#1117

Merged

Add plus signs to acq field PennLINC/CuBIDS#436

Draft

Remi-Gau approved these changes Feb 24, 2025

View reviewed changes

tsalo merged commit 93be74e into bids-standard:master Feb 25, 2025
24 of 25 checks passed

tsalo deleted the plus-sign-labels branch February 25, 2025 15:34

tsalo mentioned this pull request Feb 25, 2025

Allow + symbol in label entities nipreps/niworkflows#926

Merged

marcelzwiers added a commit to Donders-Institute/bidscoin that referenced this pull request Jun 17, 2025

Allow plus signs in labels (bids-standard/bids-specification#1926)

1d4dbe0

effigies pushed a commit to yarikoptic/BIDS-examples that referenced this pull request Aug 4, 2025

Adjust create_synthethic_ds.sh to use 2 tasknames with +

3aca99e

Ref: bids-standard/bids-specification#1926

Remi-Gau mentioned this pull request Aug 17, 2025

[ENH] update label description to mention it may containt '+' bids-standard/bids-website#692

Merged

[ENH] Allow plus signs in labels #1926

[ENH] Allow plus signs in labels #1926

Uh oh!

Conversation

tsalo commented Sep 20, 2024

Uh oh!

tsalo commented Sep 20, 2024

Uh oh!

effigies commented Sep 20, 2024

Uh oh!

tsalo commented Sep 20, 2024

Uh oh!

oesteban left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yarikoptic Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

effigies Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

yarikoptic Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

yarikoptic commented Sep 23, 2024

Uh oh!

yarikoptic commented Sep 23, 2024

Uh oh!

Uh oh!

Remi-Gau left a comment

Choose a reason for hiding this comment

Uh oh!

tsalo commented Sep 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Remi-Gau commented Sep 28, 2024

Uh oh!

tsalo commented Sep 28, 2024

Uh oh!

codecov bot commented Jan 23, 2025

Codecov Report

Uh oh!

yarikoptic left a comment

Choose a reason for hiding this comment

Uh oh!

tsalo commented Feb 5, 2025

Uh oh!

yarikoptic commented Feb 5, 2025

Uh oh!

tsalo commented Feb 10, 2025

Uh oh!

tsalo commented Feb 24, 2025

Uh oh!

tsalo commented Feb 24, 2025

Uh oh!

Uh oh!

Uh oh!

tsalo commented Sep 28, 2024 •

edited

Loading