[WIP] introduce BIDSLayoutV2

[erdalkaraca]

In agreement with @gkiar, the following approach allows for a better migration to pybids-core:

• introduce a new interface BIDSLayoutV2 which uses ancpBIDS to mimic functionality from the BIDSLayout legacy interface
• ancpBIDS will be dynamically imported at run-time and raise a warning if it is not installed
• users can decide which interface to use and experiment with BIDSLayoutV2 without breaking their existing code

Once we get sufficient user feedback such as bugs or critical implementation gaps, the BIDSLayoutV2 can be renamed to BIDSLayout while the old implementation may exist as BIDSLayoutLegacy within the next releases.

[erdalkaraca]

Note: there is a new test module: test_layout_v2.py

I have not yet fixed all unit tests in that module as this is work-in-progress

[codecov[bot]]

Codecov Report

Base: 86.24% // Head: 83.96% // Decreases project coverage by -2.28% :warning:

Coverage data is based on head (7f08cdb) compared to base (de95cb5). Patch coverage: 12.09% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #863      +/-   ##
==========================================
- Coverage   86.24%   83.96%   -2.29%     
==========================================
  Files          32       33       +1     
  Lines        3904     4028     +124     
  Branches      947      966      +19     
==========================================
+ Hits         3367     3382      +15     
- Misses        346      455     +109     
  Partials      191      191              

Impacted Files	Coverage Δ
bids/layout/layout_v2.py	7.62% <7.62%> (ø)
bids/layout/__init__.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[adelavega]

Awesome. I think a full integration test using BIDSLayoutV2 would be most informative.

That is, is those of use that use BIDSLayout heavily try to use BIDSLayoutV2 instead in our more complex apps. I will try to set aside some time for this soon.

[gkiar]

Hey @erdalkaraca are you around this week to discuss?

[effigies]

To me this feels like a big enough change to warrant a 1.0 release when we can make it happen. I won't have any time to work on this until at the absolute earliest August 22, but I do think this is a pretty important thing to push on as people are able.

Could have a ~30 minute call next week if people are around and want to discuss strategy.

[erdalkaraca]

Depends on https://github.com/ANCPLabOldenburg/ancp-bids/issues/60

[adelavega]

Update: all tests in test_layout_on_example are passing w/ ancpbids.

[adelavega]

Additional observations from trying to test functionality in other modules:

Missing functionality:

• BIDSLayoutFile.get_df
  • get_dict
  • get_entities
  • path and relpath (ancpbids Artifact only has 'name')
• layout.regex_search
• .get function on derivatives (probably it can access in different way)
• BIDSLayout.build_path write_to_file and copy_file
• As @gkiar pointed out BIDSLayout.files has a meaning of all files that are valid (if validation=True), but this is a different meaning in ancpbids. I also recommend renaming to extra_files, and adding a shim or another way to access all files in a flat list.
• validation seems to produce slightly different results-- and one difference is pybids simply skips invalid files, ancpbids seems to throw an error. @erdalkaraca? In particular this causes issues on these files on ds005: `models/ds-005_type interceptonlyrunlevel_model.json'

There's also many functions that look like get_{entity} but are not and need to be added (I'm open to adding w/ a new, less confusing name):

• get_collections -- load variable collections

Indexing discrepancies

• participants.tsv is not indexed (as @gkiar previously said, this is ill defined in the schema)

@erdalkaraca WDYT about functions like get_collections? Should we rename them, or add exceptions to __getattr__?

[erdalkaraca]

Additional observations from trying to test functionality in other modules:

Missing functionality:

• BIDSLayoutFile.get_df

  • get_dict
  • get_entities
  • path and relpath (ancpbids Artifact only has 'name')

• layout.regex_search

• .get function on derivatives (probably it can access in different way)

• BIDSLayout.build_path write_to_file and copy_file

• As @gkiar pointed out BIDSLayout.files has a meaning of all files that are valid (if validation=True), but this is a different meaning in ancpbids. I also recommend renaming to extra_files, and adding a shim or another way to access all files in a flat list.

• validation seems to produce slightly different results-- and one difference is pybids simply skips invalid files, ancpbids seems to throw an error. @erdalkaraca? In particular this causes issues on these files on ds005: `models/ds-005_type interceptonlyrunlevel_model.json'

There's also many functions that look like get_{entity} but are not and need to be added (I'm open to adding w/ a new, less confusing name):

• get_collections -- load variable collections

Indexing discrepancies

• participants.tsv is not indexed (as @gkiar previously said, this is ill defined in the schema)

@erdalkaraca WDYT about functions like get_collections? Should we rename them, or add exceptions to __getattr__?

I am not really familiar with the variable concept, but sounds like a higher level API. What is the source of the variables that pybids extracts those from?

[adelavega]

@erdalkaraca yes it is a higher-level API that requires reading in the _events.tsv files.

It is independent than the querying/indexing functionality. I will admit in hindsight its a bit messy to have this type of functionality mixed in with the core querying functionality in the same object. That said, we could address that later in a backwards breaking change.

In the meantime, we will implement these functions to allow for deeper integration testing.

I see you already saw, but we will continue development in https://github.com/bids-standard/pybids-refactor/ so that we can have multiple PRs by different authors, and then merge that fork to pybids

[erdalkaraca]

@erdalkaraca yes it is a higher-level API that requires reading in the _events.tsv files.

It is independent than the querying/indexing functionality. I will admit in hindsight its a bit messy to have this type of functionality mixed in with the core querying functionality in the same object. That said, we could address that later in a backwards breaking change.

In the meantime, we will implement these functions to allow for deeper integration testing.

I see you already saw, but we will continue development in https://github.com/bids-standard/pybids-refactor/ so that we can have multiple PRs by different authors, and then merge that fork to pybids

It is possible to mixin the functionality from a different class to better separate APIs (monkey-patching, plugin) without a breaking change.

[adelavega]

@erdalkaraca yes it is a higher-level API that requires reading in the _events.tsv files. It is independent than the querying/indexing functionality. I will admit in hindsight its a bit messy to have this type of functionality mixed in with the core querying functionality in the same object. That said, we could address that later in a backwards breaking change. In the meantime, we will implement these functions to allow for deeper integration testing. I see you already saw, but we will continue development in https://github.com/bids-standard/pybids-refactor/ so that we can have multiple PRs by different authors, and then merge that fork to pybids

It is possible to mixin the functionality from a different class to better separate APIs (monkey-patching, plugin) without a breaking change.

Yes, that might be the way to go. Maybe your BIDSLayout object should be called BIDSBaseLayout or something to indicate its the core indexer/querying engine, vs the additional high level API we will add to make it backwards compatible.