Repository: bids-standard/pybids
Pull request #605 (2020-05-10 14:46:01)

ENH: Add default path patterns to derivatives.json
This takes the MR data types from the `bids.json` file, 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.

tyarkoni: 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).
effigies: Sure. Mostly just playing around preparing a presentation and realized we don't have them.
effigies: @Remi-Gau Will this get scooped up by the tests added in #846, or do I need to add something?
codecov[bot]: # [Codecov](https://codecov.io/gh/bids-standard/pybids/pull/605?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard) Report
> Merging [#605](https://codecov.io/gh/bids-standard/pybids/pull/605?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard) (eebf40f) into [master](https://codecov.io/gh/bids-standard/pybids/commit/9f018025b290cbe34306dc8e27a16e9ef63cb82d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard) (9f01802) will **decrease** coverage by `0.02%`.
> The diff coverage is `n/a`.

```diff
@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
- Coverage   86.29%   86.26%   -0.03%     
==========================================
  Files          35       35              
  Lines        3954     3954              
  Branches     1026     1026              
==========================================
- Hits         3412     3411       -1     
  Misses        353      353              
- Partials      189      190       +1     
```


| [Impacted Files](https://codecov.io/gh/bids-standard/pybids/pull/605?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard) | Coverage Δ | |
|---|---|---|
| [bids/layout/layout.py](https://codecov.io/gh/bids-standard/pybids/pull/605/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard#diff-Ymlkcy9sYXlvdXQvbGF5b3V0LnB5) | `88.28% <0.00%> (-0.21%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/bids-standard/pybids/pull/605?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/bids-standard/pybids/pull/605?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard). Last update [9f01802...eebf40f](https://codecov.io/gh/bids-standard/pybids/pull/605?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard).

Remi-Gau: This should be picked up automatically I think. 
Remi-Gau: I could try it locally and update the json to make sure if you want. 
effigies: @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.
Remi-Gau: Temp TODO:
- [ ] add segmentation path patterns
- [ ] add test for path building on ds000001-fmriprep example
effigies: @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.
Remi-Gau: No worries. I was just working on it but I caught up quickly. 
Remi-Gau: @effigies
Feel free to merge already if you like.
Will update segmentation related config in another PR.
Won't make it today. 

Patch:
diff --git a/bids/layout/config/derivatives.json b/bids/layout/config/derivatives.json
index cb0babc0..47c9bff0 100644
--- a/bids/layout/config/derivatives.json
+++ b/bids/layout/config/derivatives.json
@@ -49,5 +49,27 @@
             "name": "subset",
             "pattern": "subset-([a-zA-Z0-9]+)"
         }
+    ],
+
+    "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}][_space-{space}][_res-{res}][_den-{den}][_label-{label}][_desc-{desc}]_{suffix<mask>}{extension<.nii|.nii.gz|.json>|.nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<func>|func}/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}][_dir-{direction}][_run-{run}][_echo-{echo}][_space-{space}][_desc-{desc}]_{suffix<bold|cbv|phase|sbref>}{extension<.nii|.nii.gz|.json>|.nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<func>|func}/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}][_dir-{direction}][_run-{run}][_echo-{echo}][_space-{space}][_res-{res}][_den-{den}][_label-{label}][_desc-{desc}]_{suffix<mask>}{extension<.nii|.nii.gz|.json>|.nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<dwi>|dwi}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_space-{space}][_desc-{desc}]_{suffix<dwi>}{extension<bval|bvec|.json|.nii.gz|.nii>|.nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<dwi>|dwi}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_space-{space}][_res-{res}][_den-{den}][_label-{label}][_desc-{desc}]_{suffix<mask>}{extension<.nii|.nii.gz|.json>|.nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<fmap>|fmap}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_dir-{direction}][_run-{run}][_space-{space}][_desc-{desc}]_{fmap<phasediff|magnitude[12]|phase[12]|fieldmap>}{extension<.nii|.nii.gz|.json>|.nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<fmap>|fmap}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}]_dir-{direction}[_run-{run}][_space-{space}][_desc-{desc}]_{fmap<epi>}{extension<.nii|.nii.gz|.json>|.nii.gz}",
+        "[acq-{acquisition}_][ce-{ceagent}_][rec-{reconstruction}_]{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}{extension<.json>|.json}",
+        "[acq-{acquisition}_][ce-{ceagent}_][rec-{reconstruction}_][space-{space}_][res-{res}_][den-{den}_][label-{label}_][desc-{desc}_]{suffix<mask>}{extension<.nii|.nii.gz|.json>|.nii.gz}",
+        "[acq-{acquisition}_][ce-{ceagent}_][rec-{reconstruction}_][mod-{modality}_][space-{space}_][desc-{desc}_]{suffix<defacemask>}{extension<.json>|.json}",
+        "task-{task}[_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}][_dir-{direction}][_run-{run}][_echo-{echo}][_space-{space}][_desc-{desc}]_{suffix<bold|cbv|phase|sbref>}{extension<.json>|.json}",
+        "task-{task}[_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}][_dir-{direction}][_run-{run}][_echo-{echo}][_space-{space}][_res-{res}][_den-{den}][_label-{label}][_desc-{desc}]_{suffix<mask>}{extension<.nii|.nii.gz|.json>|.nii.gz}",
+        "[acq-{acquisition}_][space-{space}_][desc-{desc}_]{suffix<dwi>}{extension<.json>|.json}",
+        "[acq-{acquisition}_][space-{space}_][res-{res}_][den-{den}_][label-{label}_][desc-{desc}_]{suffix<mask>}{extension<.nii|.nii.gz|.json>|.nii.gz}",
+        "[acq-{acquisition}_][dir-{direction}_][run-{run}_][space-{space}_][desc-{desc}_]{fmap<phasediff|magnitude[1-2]|phase[1-2]|fieldmap>}{extension<.json>|.json}",
+        "[acq-{acquisition}_][ce-{ceagent}_]dir-{direction}[_run-{run}][_space-{space}][_desc-{desc}]_{fmap<epi>}{extension<.json>|.json}",
+        "task-{task}[_acq-{acquisition}][_rec-{reconstruction}][_run-{run}][_echo-{echo}][_recording-{recording}][_desc-{desc}]_{suffix<events>}{extension<.json>|.json}"
     ]
 }


Test patch:
diff --git a/bids/layout/tests/test_path_building.py b/bids/layout/tests/test_path_building.py
index 9e2838c6..e90e383c 100644
--- a/bids/layout/tests/test_path_building.py
+++ b/bids/layout/tests/test_path_building.py
@@ -55,7 +55,6 @@ reconstructs the fullpath of each file by relying on pybids config
 and compares it what's actually in the dataset
 """
 
-@pytest.mark.parametrize("scope", ["raw"])
 @pytest.mark.parametrize(
     "dataset",
     [
@@ -66,16 +65,24 @@ and compares it what's actually in the dataset
         ("qmri_mtsat"),
         ("qmri_sa2rage"),
         ("qmri_vfa"),
-        ("ds000117"),
+        ("synthetic"),
+        ("ds000001-fmriprep"),
     ],
 )
-def test_path_building_on_examples_with_derivatives(dataset, scope, bids_examples):
+def test_path_building_on_derivative_examples(dataset, bids_examples):
     layout = BIDSLayout(bids_examples / dataset, derivatives=True)
-    files = layout.get(subject=".*", datatype=".*", regex_search = True, scope=scope)
-    for bf in files:
-        entities = bf.get_entities()
-        path = layout.build_path(entities)
-        assert(path==bf.path)
+    for derivative in layout.derivatives:
+        sublayout = layout.derivatives[derivative]
+        files = sublayout.get(subject=".*", datatype=".*", regex_search=True)
+        for bf in files:
+            entities = bf.get_entities()
+
+            # Some examples include unfinalized derivatives
+            if entities["suffix"] in {"timeseries"}:  # BEP012
+                continue
+
+            path = sublayout.build_path(entities, strict=True, validate=False)
+            assert path == bf.path
 
 @pytest.mark.parametrize(
     "dataset",
@@ -100,15 +107,23 @@ def test_path_building_on_examples_with_derivatives(dataset, scope, bids_example
         ("ds000248"),
         ("ds001"),
         ("ds114"),
+        ("qmri_irt1"),
+        ("qmri_mese"),
+        ("qmri_mp2rage"),
+        ("qmri_mp2rageme"),
+        ("qmri_mtsat"),
+        ("qmri_sa2rage"),
+        ("qmri_vfa"),
+        ("ds000117"),
     ],
 )
-def test_path_building_on_examples_with_no_derivatives(dataset, bids_examples):
+def test_path_building_in_raw_scope(dataset, bids_examples):
     layout = BIDSLayout(bids_examples / dataset, derivatives=False)
-    files = layout.get(subject=".*", datatype=".*", regex_search =  True)
+    files = layout.get(subject=".*", datatype=".*", regex_search=True, scope="raw")
     for bf in files:
         entities = bf.get_entities()
         path = layout.build_path(entities)
-        assert(path==bf.path)
+        assert path == bf.path
 
 @pytest.mark.parametrize("scope", ["raw"])
 @pytest.mark.parametrize(