[FIX] Re-align the text and the schema for filenames in derivatives #1567

mguaypaq · 2023-07-31T20:10:17Z

I initially set out to add the desc entity to the filename templates for segmentations (in the 'Segmentations' subsection), since the BIDS spec says here that:

When necessary to distinguish two files that do not otherwise have a distinguishing entity, the _desc- entity SHOULD be used.

But then I noticed that this repository also contains a YAML version of the spec, and the desc entity already appears there. What's more, I noticed that the atlas filename entity has the opposite problem: it appears in the text (added in #997), but not in the YAML.

I also noticed that the [ses-<label>/] folder layer was missing from the filename templates in the text about Derivatives (probably because they can't use the nifty MACROS___make_filename_template construct), so I fixed that too.

This pull request fixes all three problems.

I'm not sure if this pull request counts as a [FIX] or a [SCHEMA]: it feels to me like a bugfix, but technically it does touch the YAML files.

(This is my first contribution, but I'll wait until it's accepted before adding myself to the Recent Contributors page on the wiki.)

This entity is present in the corresponding YAML files in the schema: src/schema/rules/files/deriv/imaging.yaml

These filename entities, which appear in the corresponding text, were added in bids-standard#997, but the schema was only updated for the JSON sidecar entities.

codecov · 2023-07-31T20:14:31Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (01be684) 87.83% compared to head (1866012) 87.83%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1567   +/-   ##
=======================================
  Coverage   87.83%   87.83%           
=======================================
  Files          16       16           
  Lines        1356     1356           
=======================================
  Hits         1191     1191           
  Misses        165      165

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

tsalo

Thank you for addressing this. I think the one lingering problem is that the set of allowed entities will depend on the extension (i.e., nifti vs. gifti vs. cifti).

I also think we're not far from being able to generate these filename templates with macros (the way we do for the raw BIDS filename templates), but your fixes are much appreciated in the meantime.

tsalo · 2023-08-02T14:02:07Z

src/derivatives/imaging.md

-            <source_entities>[_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_label-<label>]_probseg.nii.gz
+        [ses-<label>/]
+            func|anat|dwi/
+                <source_entities>[_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_label-<label>][_desc-<label>]_probseg.nii.gz


I don't think den is applicable to nifti files.

Suggested change

<source_entities>[_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_label-<label>][_desc-<label>]_probseg.nii.gz

<source_entities>[_space-<space>][_atlas-<label>][_res-<label>][_label-<label>][_desc-<label>]_probseg.nii.gz

tsalo · 2023-08-02T14:02:26Z

src/derivatives/imaging.md

-            <source_entities>[_hemi-{L|R}][_space-<space>][_atlas-<label>][_res-<label>][_den-<label>]_dseg.{label.gii|dlabel.nii}
+        [ses-<label>/]
+            anat/
+                <source_entities>[_hemi-{L|R}][_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_desc-<label>]_dseg.{label.gii|dlabel.nii}


I don't think res is applicable to gifti files, though it is applicable to cifti files.

Suggested change

<source_entities>[_hemi-{L|R}][_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_desc-<label>]_dseg.{label.gii|dlabel.nii}

<source_entities>[_hemi-{L|R}][_space-<space>][_atlas-<label>][_den-<label>][_desc-<label>]_dseg.label.gii

<source_entities>[_hemi-{L|R}][_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_desc-<label>]_dseg.dlabel.nii

Do you have any thoughts on how to represent in the template the fact that accompanying .json and .tsv files are allowed? Three options that spring to mind:

Include these file extension options on both lines, that is:

..._dseg.{label.gii|json|tsv} ..._dseg.{dlabel.nii|json|tsv}

I think this is a bit clearer, but technically redundant.

Include the file extension options only on the more permissive line, that is:

..._dseg.{label.gii} ..._dseg.{dlabel.nii|json|tsv}

Maybe this would be confusing.

Have a separate line for each of label.gii, dlabel.nii, json, tsv, I guess forbidding res only for dlabel.nii?

tsalo · 2023-08-02T14:08:58Z

src/derivatives/imaging.md

-            <source_entities>[_space-<space>][_res-<label>][_den-<label>][_label-<label>][_desc-<label>]_mask.nii.gz
+        [ses-<label>/]
+            anat|func|dwi/
+                <source_entities>[_space-<space>][_res-<label>][_den-<label>][_label-<label>][_desc-<label>]_mask.nii.gz


Suggested change

<source_entities>[_space-<space>][_res-<label>][_den-<label>][_label-<label>][_desc-<label>]_mask.nii.gz

<source_entities>[_space-<space>][_res-<label>][_label-<label>][_desc-<label>]_mask.nii.gz

I don't know if we plan to support gifti or cifti masks. If so, then this snippet will need filename templates for those as well.

I'd be happy to make this change, but then it also needs to be reflected in the YAML files. Before I can do that, though, a question:

I'm noticing that the dwi datatype also allows the file extensions .bvec and .bval. Are those relevant for masks? Or should only .json, .nii and .nii.gz be allowed?

I don'tthink .bvec and .bval would apply to masks. @mattcieslak WDYT?

I can't imagine how a mask would need bvals or bvecs

tsalo · 2023-08-02T14:10:06Z

src/derivatives/imaging.md

-            <source_entities>[_space-<space>][_atlas-<label>][_res-<label>][_den-<label>]_dseg.nii.gz
+        [ses-<label>/]
+            anat|func|dwi/
+                <source_entities>[_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_desc-<label>]_dseg.nii.gz


Suggested change

<source_entities>[_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_desc-<label>]_dseg.nii.gz

<source_entities>[_space-<space>][_atlas-<label>][_res-<label>][_desc-<label>]_dseg.nii.gz

Remi-Gau · 2023-08-08T16:39:20Z

@effigies @tsalo
before @mguaypaq spends too much time on this, do we think it will be possible in the "near" future to use the schema directly to render those filename templates?

Also to estimate how thorough we want to be on this.

effigies · 2023-08-08T17:07:27Z

I would like to put the brakes on adding atlas- for the time being. This was a very small, last-minute addition to derivatives that seemed harmless at the time, but one result from the June 2023 meeting in Copenhagen was the realization that it is a very narrow definition of atlas. @melanieganz is going to be proposing changing its name to parc- (for "parcellation") in this narrow context to free up atlas- to be used in the sense of BEP38, and that's a discussion that needs to be had out (i.e., is anybody actually using atlas- as it currently is stated, or can we "safely" sweep it under the rug).

I would be pro-converting these to filename templates like the rest. It will need a new macro, since <pipeline_name> and <source_entities> won't be handled by the current one.

I don't know if we want to do something like:

meta.derivatives.masks.vol_mask:
  suffixes:
    - mask
  extensions:
    - .nii
    - .nii.gz
    - .json
  entities:
    space: optional
    resolution: optional
    label: optional
    description: optional

meta.derivatives.masks.surf_mask:
  suffixes:
    - mask
  extensions:
    - .label.gii
    - .json
  entities:
    space: optional
    density: optional
    label: optional
    description: optional

meta.derivatives.masks.volsurf_mask:
  suffixes:
    - mask
  extensions:
    - .dlabel.nii
    - .json
  entities:
    space: optional
    resolution: optional
    density: optional
    label: optional
    description: optional

rules.files.deriv.imaging.func_mask:
  $ref: rules.files.raw.func.func
  suffixes:
    $ref: meta.derivatives.masks.vol_mask.suffixes
  entities:
    $ref: rules.files.raw.func.func.entities
    $ref: meta.derivatives.masks.vol_mask.entities

That would allow us to write the macro to work on meta.derivatives.masks.* instead of on rules.files.deriv.imaging.*_mask, which will be fully expanded by the time we get to the rendering code.

Remi-Gau · 2023-12-22T10:16:37Z

switching to draft PR

mguaypaq added 3 commits July 31, 2023 15:09

Add desc entity to filename templates in the text

3c1ec6e

This entity is present in the corresponding YAML files in the schema: src/schema/rules/files/deriv/imaging.yaml

Add session layer to filename templates in the text

da05c04

Add atlas entity to the schema for segmentations

1866012

These filename entities, which appear in the corresponding text, were added in bids-standard#997, but the schema was only updated for the JSON sidecar entities.

mguaypaq requested review from tsalo, erdalkaraca and effigies as code owners July 31, 2023 20:10

tsalo requested changes Aug 2, 2023

View reviewed changes

mguaypaq mentioned this pull request Aug 3, 2023

Add corrected labels neuropoly/data-management#248

Closed

mguaypaq mentioned this pull request Oct 27, 2023

Strategy to re-use existing binary ground truth for enriching contrast-agnostic model sct-pipeline/contrast-agnostic-softseg-spinalcord#84

Closed

Remi-Gau marked this pull request as draft December 22, 2023 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Re-align the text and the schema for filenames in derivatives #1567

[FIX] Re-align the text and the schema for filenames in derivatives #1567

mguaypaq commented Jul 31, 2023

codecov bot commented Jul 31, 2023

tsalo left a comment

tsalo Aug 2, 2023

tsalo Aug 2, 2023

tsalo Aug 2, 2023

tsalo Aug 2, 2023

mguaypaq Aug 8, 2023

tsalo Aug 2, 2023

mguaypaq Aug 8, 2023

tsalo Aug 11, 2023

mattcieslak Aug 11, 2023

tsalo Aug 2, 2023

Remi-Gau commented Aug 8, 2023

effigies commented Aug 8, 2023

Remi-Gau commented Dec 22, 2023

	<source_entities>[_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_label-<label>][_desc-<label>]_probseg.nii.gz
	<source_entities>[_space-<space>][_atlas-<label>][_res-<label>][_label-<label>][_desc-<label>]_probseg.nii.gz

	<source_entities>[_hemi-{L\|R}][_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_desc-<label>]_dseg.{label.gii\|dlabel.nii}
	<source_entities>[_hemi-{L\|R}][_space-<space>][_atlas-<label>][_den-<label>][_desc-<label>]_dseg.label.gii
	<source_entities>[_hemi-{L\|R}][_space-<space>][_atlas-<label>][_res-<label>][_den-<label>][_desc-<label>]_dseg.dlabel.nii

	<source_entities>[_space-<space>][_res-<label>][_den-<label>][_label-<label>][_desc-<label>]_mask.nii.gz
	<source_entities>[_space-<space>][_res-<label>][_label-<label>][_desc-<label>]_mask.nii.gz

[FIX] Re-align the text and the schema for filenames in derivatives #1567

Are you sure you want to change the base?

[FIX] Re-align the text and the schema for filenames in derivatives #1567

Conversation

mguaypaq commented Jul 31, 2023

codecov bot commented Jul 31, 2023

Codecov Report

tsalo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Aug 8, 2023

effigies commented Aug 8, 2023

Remi-Gau commented Dec 22, 2023