[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.

yarikoptic · 2024-09-30T22:26:54Z

src/appendices/entity-table.md

@@ -12,7 +12,7 @@ For example, if a file has an acquisition and reconstruction label, the
 acquisition entity must precede the reconstruction entity.
 REQUIRED and OPTIONAL entities for a given file type are denoted;
 empty cells imply that entities MUST NOT be specified.
-Entity formats indicate whether the value is alphanumeric
+Entity formats indicate whether the value is alphanumeric (potentially including `+` character(s))


just for uniformity

Suggested change

Entity formats indicate whether the value is alphanumeric (potentially including `+` character(s))

Entity formats indicate whether the value is alphanumeric (and possibly including `+` character(s))

or should we use some other similar phrase?

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 Outdated 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

Fix regex for _run index in a test_validator.py

a096cba

yarikoptic reviewed Sep 30, 2024

View reviewed changes

yarikoptic mentioned this pull request Oct 2, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Allow plus signs in labels #1926

[ENH] Allow plus signs in labels #1926

tsalo commented Sep 20, 2024

tsalo commented Sep 20, 2024

effigies commented Sep 20, 2024

tsalo commented Sep 20, 2024

oesteban left a comment

yarikoptic Sep 23, 2024

effigies Sep 23, 2024

yarikoptic Sep 23, 2024

yarikoptic commented Sep 23, 2024

yarikoptic commented Sep 23, 2024

Remi-Gau left a comment

tsalo commented Sep 28, 2024 •

edited

Loading

Remi-Gau commented Sep 28, 2024

tsalo commented Sep 28, 2024

yarikoptic Sep 30, 2024

	Entity formats indicate whether the value is alphanumeric (potentially including `+` character(s))
	Entity formats indicate whether the value is alphanumeric (and possibly including `+` character(s))

[ENH] Allow plus signs in labels #1926

Are you sure you want to change the base?

[ENH] Allow plus signs in labels #1926

Conversation

tsalo commented Sep 20, 2024

tsalo commented Sep 20, 2024

effigies commented Sep 20, 2024

tsalo commented Sep 20, 2024

oesteban left a comment

Choose a reason for hiding this comment

yarikoptic Sep 23, 2024

Choose a reason for hiding this comment

effigies Sep 23, 2024

Choose a reason for hiding this comment

yarikoptic Sep 23, 2024

Choose a reason for hiding this comment

yarikoptic commented Sep 23, 2024

yarikoptic commented Sep 23, 2024

Remi-Gau left a comment

Choose a reason for hiding this comment

tsalo commented Sep 28, 2024 • edited Loading

Remi-Gau commented Sep 28, 2024

tsalo commented Sep 28, 2024

yarikoptic Sep 30, 2024

Choose a reason for hiding this comment

tsalo commented Sep 28, 2024 •

edited

Loading