Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
- Coverage 95.15% 87.86% -7.30%
==========================================
Files 7 11 +4
Lines 495 692 +197
==========================================
+ Hits 471 608 +137
- Misses 24 84 +60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kaitj
left a comment
kaitj
left a comment
There was a problem hiding this comment.
Been mostly playing around with this compatibility layer and haven't had a chance to look too closely at the code yet. It's been a while since I've looked at pybids so I may be misremembering or ignoring functionality.
I think the main divergence that I've seen so far is the ability to query non-standard entities. It's something that has popped up in a few projects with b2t, with different solutions proposed. I think for compatibility (and correct me if I am misremembering how pybids works), it might make sense to flatten the extra_entities into its own columns before / after it has been converted to a dataframe so those entities can be queried directly. This came up in testing indexing the ds000001-fmriprep dataset, where specific transforms couldn't be queried because of from (which may also be a keyword problem) and to entities.
Some minor things that popped up:
- the entity value type, though I think this is more related to the schema dependency (e.g.
sub-01_run-01islayout.get(subject="01", run=1)instead oflayout.get(subject="01", run="01")- probably just me misremembering how pybids works) - how or if validation should be performed for a dataset. I can't recall now if there was a reason for not including dataset validation (only entity validation) in b2t.
|
|
I've migrated most of the relevant pybids unit tests over and will add them here tomorrow. |
💯- I think that makes sense too. @nx10, I'll open an issue in the relevant repo; it's might be useful for the validator to be a niwrap tool as well. |
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Removed the validate argument from layout initialization. Passing this previously had no effect as it wasn't used anywhere downstream nor did it throw any message about it. With this change, users can still pass the `validate` when initializing BIDSLayout without it doing anything since the method accepts **kwargs
pybids doesn't have a `.df` attribute to BIDSLayout, instead relying on a `to_df()` method. To avoid breaking this and making it more plug-and-play, add a method to mimic pybids behaviour and return the attribute value
Previous flattening, via the `.flatten()` method did not flatten the extra entities due to the type of the column. Expected StructType but got MapType instead. This commit: - Adds a method, `_flatten_extra_entities`, that performs the flattening, and adds it to the entity map that is used elsewhere, while moving the root and path columns to the end of the table. The latter is not strictly necessary, but moved it to keep the entity columns together. - Removes the TODO note, as "extra_entities" is no longer a column that needs to be considered
Update pybids compatibility flattening
|
@kaitj Merged your other PR and addressed other comments; I think we're ready for a final review before merge |
4 participants
This is the PR that captures and migrates the development taken place at the nipreps hackathon in this repository. A usage guide showing the compatibility with
pybids.BIDSLayoutcan be found here: https://childmindresearch.github.io/b2t-pybids/.In particular, the approach taken here:
pybidsto identify the common patterns we should expose and support.b2ttable structure that provides the same convenience utilities, allowing a drop-in replacement for the vast majority of users.