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

Possibly too BIDS-y? #40

Closed
effigies opened this issue Oct 9, 2019 · 2 comments
Closed

Possibly too BIDS-y? #40

effigies opened this issue Oct 9, 2019 · 2 comments

Comments

@effigies
Copy link
Member

effigies commented Oct 9, 2019

I'm looking at the changes since the last release (0.1.1...master), and this stands out to me:

https://github.com/poldracklab/sdcflows/blob/a3a9978ec7e665eb278e53015f0a1c8902e563da/sdcflows/workflows/base.py#L144-L149

It looks like we're moving from passing a dictionary of fieldmaps and metadata to passing a PyBIDS BIDSFile object that provides access to these things.

I am concerned that we are tying the methods, which are general, too closely to a BIDS organizational scheme. While I think ingesting BIDS datasets is good, this tool will almost never be used in isolation, because you aren't generally going to want to susceptibility-correct non-motion-corrected files. From the perspective of benchmarking and making comparisons, too much BIDS integration may reduce flexibility, as well.

There's also a concern with relying on PyBIDS data structures like BIDSFile. From FitLins work, I can tell you that keeping in sync with the PyBIDS API can be difficult.

I think an interface or function to populate the available fieldmaps and metadata from BIDS is a good thing to provide, but I would think that it would be ideal if the actual workflows simply worked with nipype inputs and some vanilla Python data structures (e.g., dict).

Apologies for not bringing this up on the relevant PRs. I haven't been following this repo as closely as I should.

This is partly inspired by Neurostars #5207, which asks about the selection process as an isolable tool. By tying too closely to BIDS, we may be making this less usable in isolation.

@oesteban
Copy link
Member

It looks like we're moving from passing a dictionary of fieldmaps and metadata to passing a PyBIDS BIDSFile object that provides access to these things.

I am concerned that we are tying the methods, which are general, too closely to a BIDS organizational scheme. While I think ingesting BIDS datasets is good, this tool will almost never be used in isolation, because you aren't generally going to want to susceptibility-correct non-motion-corrected files. From the perspective of benchmarking and making comparisons, too much BIDS integration may reduce flexibility, as well.

The change is only for the workflow that orchestrates the available fieldmaps. I've been back and forth with this - whether it should be part of fMRIPrep's codebase or offered here for other tools to reuse.

Right now it is probably an overkill but in the near future we can think of averaging several solutions or allowing more complex IntendedFor prescriptions.

This is partly inspired by Neurostars #5207, which asks about the selection process as an isolable tool. By tying too closely to BIDS, we may be making this less usable in isolation.

When attempting to use in isolation, one should directly access the appropriate workflow. I don't think this particular change affects that.

There's also a concern with relying on PyBIDS data structures like BIDSFile. From FitLins work, I can tell you that keeping in sync with the PyBIDS API can be difficult.

Yes, I'm also concerned with this. Perhaps, I accepted this friction because the BIDSFile object is not to be passed down the line to subworkflows.

I think an interface or function to populate the available fieldmaps and metadata from BIDS is a good thing to provide, but I would think that it would be ideal if the actual workflows simply worked with nipype inputs and some vanilla Python data structures (e.g., dict).

I'll try to think this through, it might be possible to find a common ground where the interaction with PyBIDS can be scoped into a function that returns the fieldmap correction schedule with vanilla Python structure.

oesteban added a commit to oesteban/sdcflows that referenced this issue Nov 20, 2019
Following nipreps#40, rolls back the changes most involved with PyBIDS also
making the API evolve less abruptly.

The PR builds on top of nipreps#52, and closes nipreps#16.

This PR does not address #20, nor nipreps#19 and nipreps#21 although it is setting the
necessary stepping stones.
oesteban added a commit to oesteban/sdcflows that referenced this issue Nov 20, 2019
This PR:
  - [x] Refines the work in nipreps#53 addressing nipreps#40.
  - [x] Adds tests to cover the orchestration workflow.
  - [x] Fixes nipreps#7 (added regression tests for this bug).
  - [x] Updates the interface of phdiff workflows: removes the metadata
        input, which was defined for this workflow solely.
  - [x] Makes it trivial to extend the phdiff workflow to phase1/phase2
        workflows (nipreps#15).
oesteban added a commit to oesteban/sdcflows that referenced this issue Nov 20, 2019
This PR:
  - [x] Refines the work in nipreps#53 addressing nipreps#40.
  - [x] Adds tests to cover the orchestration workflow.
  - [x] Fixes nipreps#7 (added regression tests for this bug).
  - [x] Updates the interface of phdiff workflows: removes the metadata
        input, which was defined for this workflow solely.
  - [x] Makes it trivial to extend the phdiff workflow to phase1/phase2
        workflows (nipreps#15).
oesteban added a commit that referenced this issue Nov 23, 2019
This PR:
  - [x] Refines the work in #53 addressing #40.
  - [x] Adds tests to cover the orchestration workflow.
  - [x] Fixes #7 (added regression tests for this bug).
  - [x] Updates the interface of phdiff workflows: removes the metadata
        input, which was defined for this workflow solely.
  - [x] Makes it trivial to extend the phdiff workflow to phase1/phase2
        workflows (#15).
@oesteban
Copy link
Member

The preferred interface now is lists of pairs (str path, metadata dict). Please reopen if there is something else to do here. SDCFlows also provides a convenience method to generate these from a pyBIDS get_fieldmap() call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants