Skip to content

Conversation

mguaypaq
Copy link
Contributor

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

mguaypaq added 3 commits July 31, 2023 15:09
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
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.65%. Comparing base (a7dd34a) to head (eebdaf6).
⚠️ Report is 350 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1567   +/-   ##
=======================================
  Coverage   82.65%   82.65%           
=======================================
  Files          17       17           
  Lines        1534     1534           
=======================================
  Hits         1268     1268           
  Misses        266      266           

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

tsalo
tsalo previously requested changes Aug 2, 2023
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

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.

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Aug 8, 2023

@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
Copy link
Collaborator

effigies commented Aug 8, 2023

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
Copy link
Collaborator

switching to draft PR

@Remi-Gau Remi-Gau marked this pull request as draft December 22, 2023 10:16
As pointed out by @tsalo, this entity is not applicable to nifti files.
And as pointed out by @mattcieslak, dwi masks don't need bvecs and bvals.
According to @tsalo the density entity should not be allowed for gifti files,
but I don't know how to reflect that in the yaml, so I'm leaving it as slightly
too permissive.
@mguaypaq mguaypaq marked this pull request as ready for review November 19, 2024 22:29
@mguaypaq
Copy link
Contributor Author

mguaypaq commented Nov 19, 2024

I have addressed all the feedback, so that the text and yaml line up with each other. Could I get another round of reviews?

@mguaypaq mguaypaq requested a review from tsalo November 19, 2024 22:32
Copy link
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.

LGTM

@effigies effigies dismissed tsalo’s stale review April 25, 2025 13:21

Changes applied

@effigies effigies added this to the 1.10.1 milestone May 29, 2025
Copy link
Member

@julia-pfarr julia-pfarr left a comment

Choose a reason for hiding this comment

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

@effigies can you make sure that removal of den is actually only for the niftis in this PR and then merge? I wasn't able to find out because sometimes extensions are not defined

@effigies effigies merged commit 8e175f4 into bids-standard:master Jun 12, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants