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

FIX,REF: BIDSDataGrabber #2336

Merged
merged 8 commits into from
Jan 19, 2018
Merged

FIX,REF: BIDSDataGrabber #2336

merged 8 commits into from
Jan 19, 2018

Conversation

mvdoc
Copy link
Member

@mvdoc mvdoc commented Dec 10, 2017

Fixes #2333 .

Changes proposed in this pull request

  • Moved BIDSDataGrabber to interfaces.io for consistency, and also for using IOBase as a base class (otherwise failed for circular import)
  • Fixed a bug by which if infields was set, it was populated anyway by Pybids defaults
  • BIDSDataGrabber is now subclass of IOBase instead of BaseInterface
  • Added some tests to check that outputs are populated correctly.

@satra satra added this to the 0.14.1 milestone Dec 10, 2017
Copy link
Member

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

I am very in favor of this, the api change shouldn't be a huge deal since this is a relatively new interface but I'll let a second pair of eyes merge this.

One suggestion is adding this interface to the interface level __init__ for easier import?
https://github.com/nipy/nipype/blob/master/nipype/interfaces/__init__.py#L12

"anat": {"modality": "anat"}}

# If infields is empty, use all BIDS entities
if infields is None and have_pybids:
Copy link
Member

Choose a reason for hiding this comment

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

good catch

@@ -1,147 +0,0 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

I am okay with this change - but removing this is an api change from 0.14 @satra WDYT

@effigies
Copy link
Member

@mvdoc This would be good to get fixed before 1.0 (hoping to release tomorrow).

@effigies effigies modified the milestones: 0.14.1, 1.0 Jan 18, 2018
@mvdoc
Copy link
Member Author

mvdoc commented Jan 18, 2018

!!! 1.0 !!!

OK, will fix asap

mvdoc added 3 commits January 18, 2018 18:00
* 'master' of github.com:nipy/nipype:
  fix error type
  remove install_aliases
  sty
  fix tests
  refactoring tests
  update changes
  [MAINT] Cleanup engine/base
@mvdoc
Copy link
Member Author

mvdoc commented Jan 18, 2018

OK as for @mgxd's comment I added the interface to __init__.py. Let me know @effigies

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM

@effigies effigies merged commit 2649d78 into nipy:master Jan 19, 2018
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.

None yet

4 participants