Implement `BidsPartialComponent` #284

pvandyken · 2023-04-28T21:37:38Z

Resolves #243.

For a play-by-play of what I did, the commits are quite detailed and fairly well organized.

For a high level overview:

The new classes

BidsComponent is conceived of as a multi-selectable dict (sort of... actually right now only __getitem__ is implemented). If a tuple of entities is selected, you get back a BidsPartialComponent which has a ZipList but no path or name. BidsComponent is actually just a subclass of BidsPartialComponent.

If a single entity is selected from BidsComponent (or BidsPartialComponent), you get a BidsComponentRow. This represents a single entity and its values, and it's implemented as a ImmutableList (sort of a UserTuple). It has the wildcards and entities properties, but instead of returning dicts, they directly return the {str} or ['list', 'of', 'values'] associated with the entity. This is to ensure symmetry between the following access modes:

inputs['bold']['subject'].wildcards == inputs['bold'].wildcards['subject']

Note that the preferred style would still be inputs['bold'].wildcards['subject'], so this aspect of the new implementation is largely for consistency and to make everything more fail-proof.

On the other hand, BidsComponentRow.zip_lists does still return a dict mapping the one entity to its ordered entity list. This break in symmetry is motivated by the new Expandable protocol I established:

Expandable classes have an expand method, a filter method, and a zip_lists property or attribute. The signatures of these items must be as found in BidsPartialComponent. Thus, zip_lists in BidsComponentRow needs to return a dict in order to meet the protocol. In return, we're able to cleanly test all of three of the current classes using the exact same test code.

Furthermore, because BidsComponentRow is already implemented as a list with immediate access to the values, any call from the user to a symmetrically defined zip_lists would be completely redundant. It would give only a less powerful version of the list-like object the user already has.

The documentation

I broke off a bit from our completely autogenerated documentation in the api section in order to prioritize the overview of BidsComponent. BidsPartialComponent is really mostly a repeat of BidsComponent, so none of it's members are shown, just an extended docstring overview. BidsComponentRow is rather different, so it's displayed in full.

I also moved the input_* legacy properties into a dropdown box. I suppose we could technically deprecate them, but they don't really do any harm and dropping them would completely prevent older workflows from upgrading (unlike BidsDataset.wildcards etc, which probably never had much buy-in). This way we can document them and keep them, but out of the way.

Deduplication

I decided that zip_list deduplication should not be performed immediately when getting subcomponents and rows. This is both for simplicity, and to ensure that indexing remains consistent and clear as capabilities become more complex. So the only deduplication to be performed will be done in the expand method (since it unambiguously doesn't make sense for expand to return multiple identical paths)

Design Question

I stored the name of the entity in BidsComponentRow as .entity. The name is rather close to the .entities property, which returns something completely different (the .entities name doesn't make sense in isolation, but does if you consider the whole picture). So should we store .entity as something else?

TBD

Add zip_list deduplication to the expand methods

pvandyken · 2023-05-04T21:15:09Z

Okay, I think that finally fixes all the test failures.

Just note for posterity, when combining MockerFixture with a hypothesis test, you need to call mocker.stopall() at the very beginning of the test so that all the mocks are removed, otherwise you get very strange errors. This is even if you only run one example, because if the example fails, hypothesis still does multiple runs of the test to try to shrink the example.

tkkuehn

The code basically looks good to me, just a few changes for the docstrings (some of which are a matter of opinion and optional)

snakebids/utils/utils.py

snakebids/core/datasets.py

kaitj

A couple of spelling things found, but pending resolution of Tristan's review, think this one may be good to go.

Re: your design question - what would be the alternative?

snakebids/core/datasets.py

pvandyken · 2023-05-09T18:06:43Z

Re: your design question - what would be the alternative?

Not sure, guess I wanted to see if either of you would object to the status quo

tkkuehn · 2023-05-10T14:40:45Z

Should we store .entity as something else?

Sorry, forgot to specifically address this as part of my first review. I'd also be happy with entity_name, which I guess would slightly more precisely describe what part of the entity this attribute represents (i.e. the long-form name as opposed to the entity key or value). I think I'm fine with just calling it entity though.

pvandyken · 2023-05-10T15:46:21Z

Sorry, forgot to specifically address this as part of my first review. I'd also be happy with entity_name, which I guess would slightly more precisely describe what part of the entity this attribute represents (i.e. the long-form name as opposed to the entity key or value). I think I'm fine with just calling it entity though.

Okay, I'm just going to keep it then.

pvandyken · 2023-06-07T02:45:26Z

I implemented the requested changes and the __spec argument on filter. In addition:

I changed the filter api to Iterable. This makes it more flexible for use (not a breaking change), and in principle, safer in a snakemake environment where we don't have static type checking (the previous version ate iterables without filtering correctly). All the tests now use consumable iterators instead of lists.
I went ahead and made paths in all the expand methods a position-only argument __paths. This is technically a breaking change, but I highly doubt anyone is referring to it via the keyword currently. My rationale is
1. Accessing it via keyword makes it semantically confusing with **wildcards
2. paths is technically a valid wildcard name, and we're taking up that namespace for no good reason. Of course, snakemake's expand does the same thing with filepatterns and func, and this is something we can address later, but in the meantime we should keep our own functions clean.

kaitj

lgtm pending merge conflict fix

Note that we don't use importlib_resources, as it caused problems on github actions

Copies all the methods from tuple, including hashable (which only works if all of the contents are hashable). Does not support mutation

Represents the expand(), filter(), and zip_lists attributes

A generic interface supporting a key type, a value type for single keys, and a seperate value type for tuple keys

The new BidsComponent only contains code relevant to the name and path Change constructor arguments of both classes into keyword-only, as the order of the arguments changed anyway, and kw_only increases future clarity and robustness

Represents a single selected entity from a BidsComponent. Implemented as an Immutable list, so supports all the methods and abilities of a tuple. .wildcards and .entities both directly return the `{wildcard}` string or list of values associated with the entity, but zip_lists still returns a dict of entity->values to be consistent with the `Expandable` protocol

- BidsPartialComponent - BidsComponentRow - Expandable

Allow the setting of uniqueness and the culling behaviour (removing some of the generated zip_list combinations) via strategy parameters Continue to Enforce dataset uniqueness in boolean filter tests Add detailed warning to the `test_when_all_custom_paths_no_layout_indexed` test, explaining why it might go beserk on failure because of the MockerFixture being used

Ensures 'datatype', 'suffix', and 'extension' are not used as the entity in `BidsComponentRow` so that paths can be safely derived

Add sphinx_design as docs dependency to allow the hiding of deprecated syntaxes Update docstrings

Updates all the filter tests to exclusively pass consumable iterators as arguments to filter methods

__spec is an optional, kw-only argument mutually exclusive with **filters that takes str or iterable of str to apply directly to the row's entity

Ensure only unique entries used in expand tests - Now that expand deduplicates paths, we need to test with deduplicated zip_lists

This will allow the addition of a second, optional position-only argument following it: __spec. It also frees the name paths for potential wilcard names, since paths is a valid wildcard name and right now there's a conflict

pvandyken marked this pull request as ready for review May 1, 2023 20:22

github-actions bot requested review from akhanf, kaitj and tkkuehn May 3, 2023 14:42

github-actions bot assigned pvandyken May 3, 2023

pvandyken force-pushed the feat/bidscomponent/partial branch from 8756d18 to f9f74ad Compare May 4, 2023 17:33

pvandyken added the enhancement New feature or request label May 4, 2023

pvandyken force-pushed the feat/bidscomponent/partial branch from 581f837 to f908a54 Compare May 4, 2023 19:49

tkkuehn requested changes May 5, 2023

View reviewed changes

kaitj reviewed May 9, 2023

View reviewed changes

snakebids/core/datasets.py Outdated Show resolved Hide resolved

snakebids/core/datasets.py Outdated Show resolved Hide resolved

tkkuehn mentioned this pull request Jun 2, 2023

BidsDataset.filter #241

Open

pvandyken force-pushed the feat/bidscomponent/partial branch from 247a99b to 1067740 Compare June 7, 2023 02:17

pvandyken requested review from tkkuehn and kaitj June 7, 2023 02:39

pvandyken force-pushed the feat/bidscomponent/partial branch from 1067740 to 0858f9a Compare June 7, 2023 19:38

tkkuehn approved these changes Jun 12, 2023

View reviewed changes

pvandyken force-pushed the feat/bidscomponent/partial branch from 0858f9a to 23c5c6b Compare June 13, 2023 13:28

kaitj approved these changes Jun 13, 2023

View reviewed changes

pvandyken force-pushed the feat/bidscomponent/partial branch from 23c5c6b to 2b18a0f Compare June 13, 2023 21:19

pvandyken force-pushed the feat/bidscomponent/partial branch from 2b18a0f to b019748 Compare June 22, 2023 15:55

pvandyken added 5 commits June 22, 2023 12:17

Include bids_tags.json in the fakefs

91315a3

Note that we don't use importlib_resources, as it caused problems on github actions

Correctly reset MockerFixture at beginning of test

8041fef

Implement Immutable class

cd7f098

Copies all the methods from tuple, including hashable (which only works if all of the contents are hashable). Does not support mutation

Add Expandable protocol

7c8a6c8

Represents the expand(), filter(), and zip_lists attributes

Implement MultiSelectable protocol

7a446d1

A generic interface supporting a key type, a value type for single keys, and a seperate value type for tuple keys

pvandyken added 13 commits June 22, 2023 12:18

Split BidsComponent into BidsPartialComponent

629ba28

The new BidsComponent only contains code relevant to the name and path Change constructor arguments of both classes into keyword-only, as the order of the arguments changed anyway, and kw_only increases future clarity and robustness

Add strategies for New datatypes

8508b6f

- BidsPartialComponent - BidsComponentRow - Expandable

Seperate wildcard dict creation into own function

0a3fa7a

Adapt expand tests to work on all expandables

2ffa172

Implement BidsPartialComponent getitem and tests

b1e1d4c

Add path_safe as option to BidsComponentRow strat

cd4722a

Ensures 'datatype', 'suffix', and 'extension' are not used as the entity in `BidsComponentRow` so that paths can be safely derived

Update documentation for partial components

e2d3126

Add sphinx_design as docs dependency to allow the hiding of deprecated syntaxes Update docstrings

Expand filtering API to allow Iterator filters

441bee9

Updates all the filter tests to exclusively pass consumable iterators as arguments to filter methods

Implement __spec in BidsComponentRow.filter

e95be47

__spec is an optional, kw-only argument mutually exclusive with **filters that takes str or iterable of str to apply directly to the row's entity

Implement path deduplication in expand

7a53418

Ensure only unique entries used in expand tests - Now that expand deduplicates paths, we need to test with deduplicated zip_lists

Make paths in expand position-only

369c8e5

This will allow the addition of a second, optional position-only argument following it: __spec. It also frees the name paths for potential wilcard names, since paths is a valid wildcard name and right now there's a conflict

pvandyken force-pushed the feat/bidscomponent/partial branch from b019748 to 369c8e5 Compare June 22, 2023 16:29

pvandyken merged commit 89c9fd9 into khanlab:main Jun 22, 2023

pvandyken deleted the feat/bidscomponent/partial branch June 22, 2023 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `BidsPartialComponent` #284

Implement `BidsPartialComponent` #284

pvandyken commented Apr 28, 2023 •

edited

Loading

pvandyken commented May 4, 2023

tkkuehn left a comment

kaitj left a comment

pvandyken commented May 9, 2023

tkkuehn commented May 10, 2023

pvandyken commented May 10, 2023

pvandyken commented Jun 7, 2023

kaitj left a comment •

edited

Loading

Implement BidsPartialComponent #284

Implement BidsPartialComponent #284

Conversation

pvandyken commented Apr 28, 2023 • edited Loading

The new classes

The documentation

Deduplication

Design Question

TBD

pvandyken commented May 4, 2023

tkkuehn left a comment

Choose a reason for hiding this comment

kaitj left a comment

Choose a reason for hiding this comment

pvandyken commented May 9, 2023

tkkuehn commented May 10, 2023

pvandyken commented May 10, 2023

pvandyken commented Jun 7, 2023

kaitj left a comment • edited Loading

Choose a reason for hiding this comment

Implement `BidsPartialComponent` #284

Implement `BidsPartialComponent` #284

pvandyken commented Apr 28, 2023 •

edited

Loading

kaitj left a comment •

edited

Loading