Improving DataGrabber patterns

[synchon]

With the introduction of template space definitions in #252 and #268, the use of PatternDataladDataGrabber via YAML is broken as of 9316bca due to space awareness. The space key is added in get_item method of the DataGrabber and not defined in the constructor. The space key is important for every other component downstream and not having that breaks the pipeline. The solution I propose is not backwards compatible as it re-defines the structure of the patterns parameter, but in my opinion the cleanest and leanest way:

Old:

Python:

patterns = {
    "BOLD": "...",
    "T1w": "...",
}

YAML:

patterns:
  BOLD: "..."
  T1w: "..."

New:

Python:

patterns = {
    "BOLD": {
        "pattern": "...",
        "space": "...",
    },
    "T1w": {
        "pattern": "...",
        "space": "...",
    },
}

YAML:

patterns:
  BOLD:
    pattern: "..."
    space: "..."
  T1w:
    pattern: "..."
    space: "..."

I'm happy to discuss alternatives and adapt my solution.

[LeSasse]

I think a similar problem exists for when trying to add a pattern for a BOLD_mask. For example I tried to use PatternDataladDatagrabber in the YAML to make a AOMIC datagrabber that is not the in-built one, and it didn't work properly, because you need to add this mask_item key later on (see:

junifer/junifer/datagrabber/aomic/piop1.py

Line 197 in 9316bca

out["BOLD"]["mask_item"] = "BOLD_mask"

)

Maybe something like:

patterns:
  BOLD:
    pattern: "..."
    space: "..."
    mask_item: "..."
  T1w:
    pattern: "..."
    space: "..."
    mask_item: "..."

could fix that.

[synchon]

It's a valid point for sure and I like the proposed solution as well.

[fraimondo]

Indeed I thought this issue will come soon.

We have mandatory (pattern, space) and optional mask_item elements in the PatternDataGrabber.

We first need to allow for optional parameters.

Now the question is how do we deal with space. If it's not mandatory, how do we deal with "unknown"? My take is, like @synchon proposes, to make it mandatory.

Important, we should raise an error message that gives the right information.

[LeSasse]

Yes 100% if different spaces are allowed, then explicitly providing one should be mandatory.

[synchon]

We have mandatory (pattern, space) and optional mask_item elements in the PatternDataGrabber.
We first need to allow for optional parameters.

mask_item can only be kept for the ones that need it like BOLD for example

Now the question is how do we deal with space. If it's not mandatory, how do we deal with "unknown"? My take is, like @synchon proposes, to make it mandatory.

Every "data type" has a space (not sure about VBM data though) and of course one can have something else like "tothemoon" (for example) as space if the DataGrabber author decides to not consider space as mandatory for their "data type".

Important, we should raise an error message that gives the right information.

100% agree to have error but we can't do much if the space information is incorrect in the first place, so in that case the component would raise the error. The check would improve once we have YAML validation (which I'm trying to get asap) to tackle issues like #296 .

[synchon]

Yes 100% if different spaces are allowed, then explicitly providing one should be mandatory.

With the merging of #299 , we would have a "space zoo" :P

[synchon]

If y'all don't have further points and agree with the collective proposed solution, give this comment a thumbs up for acknowledgement and I'll transfer this to an issue and then to a PR with the fix. But if you have further points, feel free to continue the discussion and don't acknowledge until you run out of points :P

[fraimondo]

I'm thinking about potential extensions like EEG, which will be done sooner or later.

So far, the datagrabber can give you any file given the pattern. E.g. BOLD_confounds, 'EEG', etc... Why do we need a "space" in these cases?

Now what I think we can do is enforce some type requirements.

E.g.: BOLD, T1w, VBM_GM, VBM_WM, etc requires "pattern" and "space". BOLD_confounds only space. EEG might require some other stuff.

[synchon]

I see your point and it's reasonable. So, BOLD_confounds has a unique key format which is mandatory for that and nothing else and extending that we have space for BOLD, T1w and so on. Not sure what would be the case for EEG.

Now what I think we can do is enforce some type requirements.

Do you mean like a mapping from "data type" to "required and optional parameters"? I was thinking this and would go perfect with JSON schema which would enable validation. This would also let us extend to EEG.

[fraimondo]

I see your point and it's reasonable. So, BOLD_confounds has a unique key format which is mandatory for that and nothing else and extending that we have space for BOLD, T1w and so on. Not sure what would be the case for EEG.

Exactly! We'll see about EEG in the future.

Now what I think we can do is enforce some type requirements.

Do you mean like a mapping from "data type" to "required and optional parameters"? I was thinking this and would go perfect with JSON schema which would enable validation. This would also let us extend to EEG.

I have no comments on implementation though. But it will be great to have something validate that for each "data type" you are passing the correct mandatory parameters, as well as checking the "optional".

E.g. what if someone passes "format" to "BOLD"? Shall we allow this? It won't be used.

[synchon]

I have no comments on implementation though. But it will be great to have something validate that for each "data type" you are passing the correct mandatory parameters, as well as checking the "optional".

Yes I'm thinking in that same line.

E.g. what if someone passes "format" to "BOLD"? Shall we allow this? It won't be used.

The validation will ensure that it won't be allowed.

[fraimondo]

+1 on my side then

[synchon]

Okay looks like we can move forward, converting it to issue, feel free to add points there.