[ENH]: Improving DataGrabber `patterns` #308

Merged

synchon merged 27 commits from refactor/dg-patterns into main

2024-04-04 13:38:56 +00:00

synchon commented

2024-04-03 16:21:17 +00:00

(Migrated from github.com)

Discussed in https://github.com/juaml/junifer/discussions/307

^{Originally posted by synchon February 26, 2024}
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.

### Discussed in https://github.com/juaml/junifer/discussions/307 <div type='discussions-op-text'> <sup>Originally posted by **synchon** February 26, 2024</sup> 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: ```python patterns = { "BOLD": "...", "T1w": "...", } ``` ### YAML: ```yaml patterns: BOLD: "..." T1w: "..." ``` ## New: ### Python: ```python patterns = { "BOLD": { "pattern": "...", "space": "...", }, "T1w": { "pattern": "...", "space": "...", }, } ``` ### YAML: ```yaml patterns: BOLD: pattern: "..." space: "..." T1w: pattern: "..." space: "..." ``` I'm happy to discuss alternatives and adapt my solution.</div>

codecov[bot] commented

2024-04-04 10:20:33 +00:00

(Migrated from github.com)

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (407d189) to head (3e89413).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #308   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            1         1           
=========================================
  Hits             1         1

Flag	Coverage Δ
docs	`100.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/308?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report All modified and coverable lines are covered by tests :white_check_mark: > Project coverage is 100.00%. Comparing base [(`407d189`)](https://app.codecov.io/gh/juaml/junifer/commit/407d18995fafb6b3b51454eead6c6a35c7ef4601?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) to head [(`3e89413`)](https://app.codecov.io/gh/juaml/junifer/pull/308?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml). > Report is 1 commits behind head on main. <details><summary>Additional details and impacted files</summary> [![Impacted file tree graph](https://app.codecov.io/gh/juaml/junifer/pull/308/graphs/tree.svg?width=650&height=150&src=pr&token=5H21JuZXMw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml)](https://app.codecov.io/gh/juaml/junifer/pull/308?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #308 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 1 1 Lines 1 1 ========================================= Hits 1 1 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/308/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [docs](https://app.codecov.io/gh/juaml/junifer/pull/308/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `100.00% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#carryforward-flags-in-the-pull-request-comment) to find out more. </details>

github-actions[bot] commented

2024-04-04 10:42:12 +00:00

(Migrated from github.com)

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-04-04 14:01 UTC

[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.7 :---: Preview removed because the pull request was closed. 2024-04-04 14:01 UTC

fraimondo (Migrated from github.com) approved these changes

2024-04-04 13:30:49 +00:00

LeSasse (Migrated from github.com) approved these changes

2024-04-04 13:36:51 +00:00

LeSasse (Migrated from github.com) left a comment

LGTM