Using dictionary operations for bids() and expand()

[Karl5766]

The problem

I'm following the snakebids tutorial and issue #209. The current way of passing arguments to bids() and expand() seem to be to use entities/wildcards found in the current BidsDataset as a base dict, and manually add a few components if needed. For example, from the tutorial

rule all:
    input:
        inputs['bold'].expand(
            bids(
                root='results',
                fwhm='{fwhm}',
                suffix='bold.nii.gz',
                **inputs['bold'].wildcards
            ),
            fwhm=config['fwhm'],
        )

There are two possible issues:

1. expand() depends only on the zip_lists attribute of a dataset. input['bold'] which may have other attributes not needed (e.g. dataset root path), suggesting separating expand() function out of BidsComponentRow/BidsPartialComponent class as an independent function bids_expand(). With this in mind, we can change the above rule all to the following:

rule all:
    input:
        bids_expand(
            bids(
                root='results',
                fwhm='{fwhm}',
                suffix='bold.nii.gz',
                **inputs['bold'].wildcards
            ),
            fwhm=config['fwhm'],
            **inputs['bold'].zip_lists
        )

2. Interfaces like BidsDataset.drop() or BidsDataset.with_entities() may be equivalent to doing some more intuitive and simpler dictionary operations over entities or wildcards.
   Assume we define a subclass MyDict of Python dict that supports the following operations:

   1. update or add entries (overwriting content of d1 with contents from d2) This is documented here
      d1 | d2
   2. we can define a function that returns a dict copy with removed entries, or define a subclass of dict and support this with a subtract '-' operation override.
      d - k # remove key k from dict d
d - [k1, k2…, kn] # subtracting an iterable of keys; i.e. remove n keys at once
   3. We can define another function or operation that returns a new dict with selected subset of keys
      d & k
d & [k1, k2…, kn]
   4. One thing to note is all entities dictionary etc. have strings as keys. To avoid typing quotes " " when initializing a dict[str, any], we can use dict() instead of {}, i.e.
      d = {'subject': '{subject}'} becomes d = dict(subject='{subject}')

To make code analysis easier, above operations 2-3 should be implemented as pure functions (with no side effects and solely depends on its input parameters), instead of modifying the dictionary caller.

This would allow reuse of variables, for example if A and B are two BIDS datasets on paths 'path/to/a' and 'path/to/b', we can define

	zipdict = generated_inputs['some_name'].zipdict
	"""
	zipdict may be something like 
	{
	    'subject': ['s1', 's2'],
	    'sample': ['sample1', 'sample2']
	}
	"""
	
	dictA = zipdict | {'root': 'path/to/a'}
	# assume we are averaging over samples in our analysis, we don't need them in the output path, so we can subtract away the 'sample' key
	dictB = (zipdict | {'root': 'path/to/b'}) - 'sample'
	
	rule R1:
	    input:
	        path = bids_expandpath(**dictA)
	    …
	    output:
	        path = bids_expandpath(**dictB)

Similarly, if we want to form wildcard paths for both input and output, we may write

	entitydict = {
	    'datatype': 'micr',
	    'stain': '{stain}'
	}  # missing some keys, but used as an example
	dictA = entitydict | {'suffix': 'source.nii'}
	dictB = entitydict | {'suffix': 'target.nii'}
	
	rule R2:
	    input:
	        path = bids(**dictA)
	    …
	    output:
	        path = bids(**dictB)

Proposed Enhancement

I would like to propose a different way to pass parameters to bids() and expand() functions, as described above.

To do so, we need to move expand() out of BidsComponentRow/BidsPartialComponent class, and add a dict subclass to support parameter concatenation, overwrite, keyset removal and subsetting.

Environment

• The version of snakebids is the main branch as of today

[pvandyken]

I'm not sure I understand the motivation of point 1. The point of the method is to reconstitute the paths forming a component, or to expand over a new template. It actually does depend on the component's path template: it's used as the template if no new template is provided.

Is there any practical benefit to getting the isolated expand function? Note that in the snakemake context, you of course already get the expand() function anyway. Also note that we're moving toward abstracting away the zip_lists property away completely, so new code patterns shouldn't rely on it

[pvandyken]

See #201 for the design logic of the expand method

[Karl5766]

For a while I was confused looking at

inputs['spim'].expand(
    bids(
        root=root,
        ...
    )
    ...
)

because I thought inputs['spim'] was the object providing the root path (by concatenating dataset path with the template path generated by bids()), but it's actually root=root that provides the root path. After looking into the source code on github I understood why, but I think having a single source of root is a more readable solution.

I guess conceptually I was thinking of inputs['spim'] as a dataset, and when expand() is called I thought it would create a list of paths under the dataset path. Other than that, two approaches do the same thing.

[pvandyken]

expand doesn't really know what a root is. Actually , components in general don't have roots.

expand just takes a path template. In the example, the bids function is being used to create the template, but you could provide a path by another means

[pvandyken]

FWIW, there is an awkwardness to the current syntax. Haven't written it up yet, but I plan on making a derive() method that takes a new root and entities like the bids function and returns a new BidsComponent with a new path template. With that, you could basically do inputs["t1w"].derive(root, desc="preproc").expand(), which I think is a little easier to read.

[pvandyken]

For point 2, I think you may be thinking of BidsComponent, not BidsDataset? The intersection operation from 2.1 is already possible with the BidsDataset because it's just a subclass of dict. I don't think that sort of intersection makes sense for BidsComponent.

2.2, 2.3, and 2.4 seem a bit strange to me, to be honest. It's certainly possible with python operator overloading, but it's generally not advisable to do mathematical operations between two different types (e.g. a BidsComponent and a string). I can't think of a single other python library that uses a convention like that.

[pvandyken]

Ah, I do see the proposal for BidsDataset.drop() now. Part of the motivation for that is the need for a pure function. It's difficult to remove keys from a python dict without modifying the dict. The .drop() method would let us achieve that without breaking the dict api

[Karl5766]

Yes, that actually should be BidsComponent. I see what you mean, overloading operations may make some type checking bugs harder to find, and usually operations are overloaded between similar types (e.g. a string buffer + str).

The operations I wrote about are similar to dictview object operations, as documented in the Python 2 documentation

dictview & other
Return the intersection of the dictview and the other object as a new set.

dictview | other
Return the union of the dictview and the other object as a new set.

dictview - other
Return the difference between the dictview and the other object (all elements in dictview that aren’t in other) as a new set.

dictview ^ other
Return the symmetric difference (all elements either in dictview or other, but not in both) of the dictview and the other object as a new set.

and Python 3 documentation. But they work on dict.keys(), dict.values() and dict.items(), but not dict itself. I thought it would be handy to be able to select keys to create a new dictionary using the same syntax, but there could be issues of type safety, and other issues if I'm not aware of.

[Karl5766]

Part of the reason I posted this discussion is that I was a bit unsure whether it would work out, and I would like to get some viewpoint from people more familiar with the codebase.

Technically the 2 changes shouldn't need to break existing interfaces. I think I will wait a bit and do some experimentation locally, if it works out well I will create a PR.

[pvandyken]

It's not a question of type safety, that's perfectly doable using type annotations. It's more a question of API design: the API should be predictable and "Pythonic". Given my experience with other libraries in python, I wouldn't expect to select keys using a subtraction operation, that's why I said I wasn't aware of other examples of this type of interface. On the other hand, something like .drop() has precedence in libraries like pandas.

It is a creative solution though and in isolation may make sense! I just don't think it makes sense in the broader python landscape.