Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REF] Refactor BIDS and CAPS reader configurations #752

Merged
merged 10 commits into from
Oct 5, 2022

Conversation

NicolasGensollen
Copy link
Member

This PR proposes to refactor the BIDS (see #658) and CAPS (see #731) reader configurations. More precisely, this PR attempts to:

  • propose a uniform format for BIDS and CAPS queries which used to be specified as strings and dictionaries respectively. Now, everything is specified as dictionaries.
  • factorize the code since both readers now expect similar data formats

Another reason, which originally motivated this refactoring is that we will pretty soon have to provide more complex BIDS input specifications than simple strings (as it is currently the case for t1-linear with T1W).

For example, in PETVolume (see #729), we need to read PET scans as inputs, but the user specifies through the CLI the tracer of interest. This means that we need to be able to dynamically pass this information to the querying engine, and not rely on static predefined queries, otherwise we will end up pretty soon with un-maintainable query registries like this:

bids_default_queries = {
        "T1w": {"datatype": "anat", "suffix": "T1w", "extension": [".nii.gz"]},
        "pet": {"datatype": "pet", "suffix": "pet", "extension": [".nii.gz"]},
        "pet_fdg": {"datatype": "pet", "suffix": "pet", "tracer": "fdg", "extension": [".nii.gz"]},
        "pet_av45": {"datatype": "pet", "suffix": "pet", "tracer": "av45", "extension": [".nii.gz"]},
     ...
    }

@@ -126,11 +150,7 @@ def add_input_task(
Workflow
An BIDS/CAPS reader based workflow.
"""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this function stands, the task added is hard-coded to be a "reader task". Maybe we can move l.153 to add_input_reading_task and make this function more generic in case we need to add another "input task" such as a provenance task.

@NicolasGensollen
Copy link
Member Author

52e0bd1 simplifies the input specifications (no need to specify the _graph_checksums field manually anymore, see nipype/pydra#584). This requires the very latest Pydra though....

@NicolasGensollen NicolasGensollen marked this pull request as ready for review September 28, 2022 12:49
@omar-rifai
Copy link
Contributor

Hi @NicolasGensollen, thanks for this PR ! there seems to be a couple of unit tests that need updating.

@omar-rifai
Copy link
Contributor

Thanks @NicolasGensollen ! The code LGTM. If no pydra release is planned we can add the dev dependency and merge.

@ghisvail
Copy link
Collaborator

ghisvail commented Oct 5, 2022

Thanks @NicolasGensollen ! The code LGTM. If no pydra release is planned we can add the dev dependency and merge.

We could add it now, check that CI passes, and later update pyproject.toml to use pydra = "^0.20" if the release happens soon enough.

@omar-rifai
Copy link
Contributor

Ok fair enough, I'll merge.

@omar-rifai omar-rifai merged commit 97b7690 into aramis-lab:dev Oct 5, 2022
@NicolasGensollen NicolasGensollen deleted the rework-caps-reader branch October 5, 2022 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants