ENH: Add default path patterns to derivatives.json #605
Conversation
|
I'm fine adding the patterns, but do we want to add tests before merging? I know it's a PITA, and I'm not suggesting you should write them right now (I'm fine leaving this open and marking as "help wanted"), but we've been bitten in the ass before by this, so I'm hesitant to merge without them. I wonder if rather than manually writing a long series of tests, we could just add a generic function that takes one of the test datasets, gets all filenames, and for each, extracts its entities and then tries to build the path and makes sure the two match (in cases where there isn't any ambiguity in the building). |
|
Sure. Mostly just playing around preparing a presentation and realized we don't have them. |
effigies
force-pushed
the
bep003/default_path_patterns
branch
from
83d6c55
to
eebf40f
Compare
Codecov Report
@@ Coverage Diff @@
## master #605 +/- ##
==========================================
+ Coverage 86.37% 86.42% +0.05%
==========================================
Files 35 35
Lines 3984 3984
Branches 1034 1034
==========================================
+ Hits 3441 3443 +2
+ Misses 352 350 -2
Partials 191 191
Continue to review full report at Codecov.
|
|
This should be picked up automatically I think. |
|
I could try it locally and update the json to make sure if you want. |
effigies
force-pushed
the
bep003/default_path_patterns
branch
from
eebf40f
to
4722519
Compare
|
@Remi-Gau Would you mind having a quick look? I'm okay merging this before Monday's release if you don't have time, but I would appreciate your eyes. |
| if {"label"} & set(entities): # BEP011 | ||
| continue |
There was a problem hiding this comment.
So that would skip the mask and segmentation as they are currently in the spec, correct?
Do you want me to add them to the config ?
There was a problem hiding this comment.
If you're up for it, sure!
bids/layout/config/derivatives.json
Outdated
| "default_path_patterns": [ | ||
| "sub-{subject}[/ses-{session}]/{datatype<anat>|anat}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}][_space-{space}][_desc-{desc}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}{extension<.nii|.nii.gz|.json>|.nii.gz}", | ||
| "sub-{subject}[/ses-{session}]/{datatype<anat>|anat}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}][_mod-{modality}][_space-{space}][_desc-{desc}]_{suffix<defacemask>}{extension<.nii|.nii.gz|.json>|.nii.gz}", | ||
| "sub-{subject}[/ses-{session}]/{datatype{anat}|anat}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}][_part-{part}][_space-{space}][_res-{res}][_den-{den}][_label-{label}][_desc-{desc}]_{suffix<mask>}{extension<.nii|.nii.gz|.json>|.nii.gz}", |
There was a problem hiding this comment.
I have kept the "part" entity in the path patterns for the mask but it is not there in the other ones: should I
- a) remove it from the mask pattern?
- b) add it to all the others?
I have a preference for "b" because it would stick closer to the spec even if this is an edge case.
There was a problem hiding this comment.
part does not make sense for a mask. A mask is binary, not complex.
There was a problem hiding this comment.
gotcha
but could it be that some other derivatives may need it?
There was a problem hiding this comment.
Minimally preprocessed files, I guess, so the ones that are just copies of raw + space/desc. Otherwise probably not. (Fieldmaps already account for it with suffix.) If someone comes up with a use case, they can open an issue. Or hopefully we move away from this onto something schema defined before they have a chance.
|
Temp TODO:
|
We had been testing the raw scope separately for datasets with and without derivatives. Now test the raw scope for all examples, then test each derivative individually. Synthetic contains some unfinalized derivatives, so make some temporary exceptions.
effigies
force-pushed
the
bep003/default_path_patterns
branch
from
b09877c
to
b3f7349
Compare
|
@Remi-Gau Tested the latest commit before pushing. Not sure if you're still working on this, but a heads up that there's a change. |
|
No worries. I was just working on it but I caught up quickly. |
|
@effigies |
This takes the MR data types from the
bids.jsonfile, and adds optional space and desc keywords. It adds desc to events.tsv. I left out the electrophysiological ones for now, as I don't know them well enough to add, and I'd rather it be done correctly later than poorly now.I also haven't added
res/den, which will need to be added soon enough, but they're not even in the entities yet.