Repository: bids-standard/pybids
Pull request #805 (2022-01-20 18:48:45)

Implement automatic detection of derivative data
Here's my implementation of #804. I couldn't find documentation of autoformatters or code quality checkers, so nothing of that sort has been applied. The changes are summarized below, with some comments following:

# Changes

BIDSLayout now checks if the root dataset is a derivative by looking for the `DatasetType` entity in `dataset_description.json` to be set to `derivative`. If it is a derivative, the appropriate config (`==['bids', 'derivatives']`) will by used for parsing. No check is made if no valid dataset_description is returned.

If `validate=True`, the derivative dataset is validated using `validate_derivative_paths()`. Bug in `validate_derivative_paths preventing()` the validation of `dataset_description.json` files without either `GeneratedBy` or `PipelineDescription` fields is fixed.

Two new tests are provided:

1) A class of tests checking that:
  a) A dataset with `desc` entities and `PipelineDescription` but no `DatasetType` is fully parsed when `validate=False`, but does not accept `desc` as a filter. It fails to find any files when `validate=True`
  b) A dataset with `DatasetType=derivative` is parsed as a derivative. `desc` entities are recognized
  c) A dataset with `DatasetType=derivative` but missing both `PipelineDescription` and `GeneratedBy` raises an error when `validate=True`. If `validate=False`, it parses as a derivative as in b)

2) Test that db-saved configs not overwritten. This could be a concern if `DatasetType` is changed between the times the db is saved and loaded. Add `DatasetType=derivative` to `ds005_derivs` test dataset

In test 2, the db is put in a fresh tmpdir rather than the already existing db-path construct as it was bugging out when multiple tests in a row used it.

# Comments

Getting this to work in a "minimally invasive" way was fairly straight forward, and I don't see a reason why this couldn't be released to a stable version. Looking further ahead though, the implementation is somewhat unideal.

The biggest problem this PR introduces is, in my opinion, a disunified way of treating derivative datasets. This shows up especially in the fact that if `validate=True`, derivative datasets added via `add_derivatives()` will be validated twice, once by `add_derivatives()` and again in the `__init__` of `BIDSLayout`. There's also now three completely separate locations where default values of config are set (in `BIDSLayout.__init__()`, `add_derivatives()`, and (`db.py:ConnectionManager.reset_database()`). I've played around with how to fix this, but it involves a certain amount of refactoring and a breaking change involving the way derivatives are internally indexed, which is well beyond the scope of this PR. Happy to leave my thoughts in a new Issue, but I fully understand that contributions involving refactoring and breaking changes aren't necessarily helpful, so you can let me know what you think.


Patch:
diff --git a/bids/layout/layout.py b/bids/layout/layout.py
index 1088cf2d..6064ad4d 100644
--- a/bids/layout/layout.py
+++ b/bids/layout/layout.py
@@ -134,6 +134,12 @@ class BIDSLayout(object):
 
         # Initialize a completely new layout and index the dataset
         if not load_db:
+            # If root dataset is a derivative, set config accordingly
+            if description and description.get("DatasetType") == "derivative":
+                if validate:
+                    validate_derivative_paths([root], self)
+                config = ["bids", "derivatives"]
+
             init_args = dict(root=root, absolute_paths=absolute_paths,
                              derivatives=derivatives, config=config)
 
diff --git a/bids/layout/validation.py b/bids/layout/validation.py
index 79aa5a9b..76331f9a 100644
--- a/bids/layout/validation.py
+++ b/bids/layout/validation.py
@@ -153,6 +153,8 @@ def validate_derivative_paths(paths, layout=None, **kwargs):
                           "``pybids upgrade`` to update your derivative "
                           "dataset.")
             pipeline_name = description["PipelineDescription"].get("Name")
+        else:
+            pipeline_name = None
         if pipeline_name is None:
             raise BIDSDerivativesValidationError(
                                 "Every valid BIDS-derivatives dataset must "


Test patch:
diff --git a/bids/layout/tests/conftest.py b/bids/layout/tests/conftest.py
index 5920edf9..e03d3349 100644
--- a/bids/layout/tests/conftest.py
+++ b/bids/layout/tests/conftest.py
@@ -54,7 +54,7 @@ def layout_ds005_multi_derivs(request, db_dir):
 
     layout = BIDSLayout(data_dir,
                         database_path=database_path)
-    deriv_dir1 = join(get_test_data_path(), 'ds005_derivs')
+    deriv_dir1 = join(get_test_data_path(), 'ds005_derivs', 'dummy')
     deriv_dir2 = join(data_dir, 'derivatives', 'events')
     layout.add_derivatives([deriv_dir1, deriv_dir2])
     return layout
diff --git a/bids/layout/tests/test_layout.py b/bids/layout/tests/test_layout.py
index be4ee4ec..1e79a63a 100644
--- a/bids/layout/tests/test_layout.py
+++ b/bids/layout/tests/test_layout.py
@@ -18,6 +18,7 @@ from bids.tests import get_test_data_path
 from bids.utils import natural_sort
 
 from bids.exceptions import (
+    BIDSDerivativesValidationError,
     BIDSValidationError,
     NoMatchError,
     TargetError,
@@ -141,6 +142,47 @@ def test_get_file(layout_ds005_derivs):
     assert not layout.get_file('/absolute/bleargh')
 
 
+class TestDerivativeAsRoot:
+    def test_dataset_without_datasettype_parsed_as_raw(self):
+        dataset_path = Path("ds005_derivs", "format_errs", "no_dataset_type")
+        unvalidated = BIDSLayout(
+            Path(get_test_data_path())/dataset_path,
+            validate=False
+        )
+        assert len(unvalidated.get()) == 4
+        with pytest.raises(ValueError):
+            unvalidated.get(desc="preproc")
+
+        validated = BIDSLayout(Path(get_test_data_path())/dataset_path)
+        assert len(validated.get()) == 1
+
+    def test_dataset_missing_generatedby_fails_validation(self):
+        dataset_path = Path("ds005_derivs", "format_errs", "no_pipeline_description")
+        with pytest.raises(BIDSDerivativesValidationError):
+            BIDSLayout(Path(get_test_data_path())/dataset_path)
+
+
+    def test_correctly_formatted_derivative_loads_as_derivative(self):
+        dataset_path = Path("ds005_derivs", "dummy")
+        layout = BIDSLayout(Path(get_test_data_path())/dataset_path)
+        assert len(layout.get()) == 4
+        assert len(layout.get(desc="preproc")) == 3
+
+    @pytest.mark.parametrize(
+        "dataset_path",
+        [
+            Path("ds005_derivs", "dummy"),
+            Path("ds005_derivs", "format_errs", "no_pipeline_description")
+        ]
+    )
+    def test_derivative_datasets_load_with_no_validation(self, dataset_path):
+        layout = BIDSLayout(
+            Path(get_test_data_path())/dataset_path,
+            validate=False
+        )
+        assert len(layout.get()) == 4
+        assert len(layout.get(desc="preproc")) == 3
+
 def test_get_metadata(layout_7t_trt):
     target = 'sub-03/ses-2/func/sub-03_ses-2_task-' \
              'rest_acq-fullbrain_run-2_bold.nii.gz'
@@ -722,3 +764,30 @@ def test_load_layout(layout_synthetic_nodb, db_dir):
     cm2 = reloaded.connection_manager
     for attr in ['root', 'absolute_paths', 'config', 'derivatives']:
         assert getattr(cm1.layout_info, attr) == getattr(cm2.layout_info, attr)
+
+
+def test_load_layout_config_not_overwritten(layout_synthetic_nodb, tmpdir):
+    modified_dataset_path = tmpdir/"modified"
+    shutil.copytree(layout_synthetic_nodb.root, modified_dataset_path)
+
+    # Save index
+    db_path = str(tmpdir / 'tmp_db')
+    BIDSLayout(modified_dataset_path).save(db_path)
+
+    # Update dataset_description.json
+    dataset_description = modified_dataset_path/"dataset_description.json"
+    with dataset_description.open('r') as f:
+        description = json.load(f)
+    description["DatasetType"] = "derivative"
+    with dataset_description.open('w') as f:
+        json.dump(description, f)
+
+    # Reload
+    db_layout = BIDSLayout(modified_dataset_path, database_path=db_path)
+    fresh_layout = BIDSLayout(modified_dataset_path, validate=False)
+    cm1 = db_layout.connection_manager
+    cm2 = fresh_layout.connection_manager
+    for attr in ['root', 'absolute_paths', 'derivatives']:
+        assert getattr(cm1.layout_info, attr) == getattr(cm2.layout_info, attr)
+
+    assert cm1.layout_info.config != cm2.layout_info.config
\ No newline at end of file
diff --git a/bids/tests/data/ds005_derivs/dummy/dataset_description.json b/bids/tests/data/ds005_derivs/dummy/dataset_description.json
index 99b91557..3112c059 100644
--- a/bids/tests/data/ds005_derivs/dummy/dataset_description.json
+++ b/bids/tests/data/ds005_derivs/dummy/dataset_description.json
@@ -4,5 +4,6 @@
       },
     "BIDSVersion": "1.0.0rc2",
     "Name": "Mixed-gambles task -- dummy derivative",
-    "ReferencesAndLinks": "Tom, S.M., Fox, C.R., Trepel, C., Poldrack, R.A. (2007). The neural basis of loss aversion in decision-making under risk. Science, 315(5811):515-8"
+    "ReferencesAndLinks": "Tom, S.M., Fox, C.R., Trepel, C., Poldrack, R.A. (2007). The neural basis of loss aversion in decision-making under risk. Science, 315(5811):515-8",
+    "DatasetType": "derivative"
 }
diff --git a/bids/tests/data/ds005_derivs/format_errs/no_dataset_type/dataset_description.json b/bids/tests/data/ds005_derivs/format_errs/no_dataset_type/dataset_description.json
new file mode 100644
index 00000000..99b91557
--- /dev/null
+++ b/bids/tests/data/ds005_derivs/format_errs/no_dataset_type/dataset_description.json
@@ -0,0 +1,8 @@
+{
+    "PipelineDescription": {
+        "Name": "dummy"
+      },
+    "BIDSVersion": "1.0.0rc2",
+    "Name": "Mixed-gambles task -- dummy derivative",
+    "ReferencesAndLinks": "Tom, S.M., Fox, C.R., Trepel, C., Poldrack, R.A. (2007). The neural basis of loss aversion in decision-making under risk. Science, 315(5811):515-8"
+}
diff --git a/bids/tests/data/ds005_derivs/format_errs/no_dataset_type/sub-01/func/sub-01_task-mixedgamblestask_run-01_desc-preproc_bold.nii.gz b/bids/tests/data/ds005_derivs/format_errs/no_dataset_type/sub-01/func/sub-01_task-mixedgamblestask_run-01_desc-preproc_bold.nii.gz
new file mode 100644
index 00000000..e69de29b
diff --git a/bids/tests/data/ds005_derivs/format_errs/no_dataset_type/sub-01/func/sub-01_task-mixedgamblestask_run-02_desc-preproc_bold.nii.gz b/bids/tests/data/ds005_derivs/format_errs/no_dataset_type/sub-01/func/sub-01_task-mixedgamblestask_run-02_desc-preproc_bold.nii.gz
new file mode 100644
index 00000000..e69de29b
diff --git a/bids/tests/data/ds005_derivs/format_errs/no_dataset_type/sub-01/func/sub-01_task-mixedgamblestask_run-03_desc-preproc_bold.nii.gz b/bids/tests/data/ds005_derivs/format_errs/no_dataset_type/sub-01/func/sub-01_task-mixedgamblestask_run-03_desc-preproc_bold.nii.gz
new file mode 100644
index 00000000..e69de29b
diff --git a/bids/tests/data/ds005_derivs/format_errs/no_pipeline_description/dataset_description.json b/bids/tests/data/ds005_derivs/format_errs/no_pipeline_description/dataset_description.json
new file mode 100644
index 00000000..b4ef3bed
--- /dev/null
+++ b/bids/tests/data/ds005_derivs/format_errs/no_pipeline_description/dataset_description.json
@@ -0,0 +1,6 @@
+{
+    "BIDSVersion": "1.0.0rc2",
+    "Name": "Mixed-gambles task -- dummy derivative",
+    "ReferencesAndLinks": "Tom, S.M., Fox, C.R., Trepel, C., Poldrack, R.A. (2007). The neural basis of loss aversion in decision-making under risk. Science, 315(5811):515-8",
+    "DatasetType": "derivative"
+}