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

ENH: BIDS Data Grabber #2174

Merged
merged 23 commits into from
Sep 25, 2017
Merged

ENH: BIDS Data Grabber #2174

merged 23 commits into from
Sep 25, 2017

Conversation

adelavega
Copy link
Contributor

@adelavega adelavega commented Sep 1, 2017

Changes proposed in this pull request

  • Added a BIDSDataGrabber Interface. This interface has a similar API to DataGrabber, and wraps pybids grabbit to query files in a BIDS project.
    • Can filter results using pybids entities with dynamic, user-defined infields
    • Can define dynamic outfields to execute multiple queries, such as grabbing both func and anat files for a subject in a given run.
  • Added a BIDS dataset (from pybids test data) to test the interface, and added tests using doctest
    • In the future this should be a git submodule

@tyarkoni
Copy link

tyarkoni commented Sep 1, 2017

Do we need to duplicate the entire BIDS dataset from pybids? Since pybids is a dependency for this module, you should be able to grab the test data from the original source. I think if you just get bids.__file__ after import bids you should be ready to go.

@adelavega
Copy link
Contributor Author

adelavega commented Sep 1, 2017

Oh yeah, good call.

Also to-do:

  • Remove pybids as requirement, make optional and check on import
  • Add to travis so tests can run (passing locally)

@effigies
Copy link
Member

effigies commented Sep 1, 2017

Personally, I think it's reasonable to add pybids as a dependency. It's pretty lightweight.

@tyarkoni
Copy link

tyarkoni commented Sep 1, 2017

Right now it is, but we will likely include pandas (and hence numpy etc.) as a dependency of pybids once we start adding transformation and models module. Not sure how people feel about that...

@codecov-io
Copy link

codecov-io commented Sep 4, 2017

Codecov Report

Merging #2174 into master will increase coverage by 0.01%.
The diff coverage is 81.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2174      +/-   ##
==========================================
+ Coverage   72.26%   72.27%   +0.01%     
==========================================
  Files        1173     1174       +1     
  Lines       58673    58738      +65     
  Branches     8442     8454      +12     
==========================================
+ Hits        42399    42453      +54     
- Misses      14916    14924       +8     
- Partials     1358     1361       +3
Flag Coverage Δ
#smoketests 72.27% <81.53%> (+0.01%) ⬆️
#unittests 69.94% <81.53%> (+0.01%) ⬆️
Impacted Files Coverage Δ
nipype/info.py 84.61% <ø> (ø) ⬆️
nipype/interfaces/io.py 53.11% <100%> (+0.03%) ⬆️
nipype/interfaces/bids_utils.py 81.25% <81.25%> (ø)
nipype/utils/logger.py 58.22% <0%> (+1.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ada8c56...0d877ee. Read the comment docs.

@adelavega
Copy link
Contributor Author

@satra comments 9/4/17: Consider adding optional and required outfields, such that an error is thrown is nothing is found for required outfields, and Undefined is passed if nothing found for optional outputs.

I'll work on getting travis & circle to work and ping you for a review.

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.

@adelavega I haven't had a chance to make a more thorough review, so here are the couple of comments I've had so far.

desc='Path to BIDS Directory.',
mandatory=True)
output_query = traits.Dict(key_trait=Str,
value_trait=traits.Dict,
Copy link
Member

Choose a reason for hiding this comment

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

Should we add key and value traits to the internal Dict trait?


class BIDSDataGrabber(BaseInterface):

""" BIDS datagrabber module that wraps around pybids to allow arbitrary
Copy link
Member

Choose a reason for hiding this comment

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

Docstring should start immediately after class line, and the contents of the docstring should only be tabbed out 4 spaces, not 8.

    """BIDS datagrabber ...
    querying of BIDS datasets.

    ...

from warnings import warn

class BIDSDataGrabberInputSpec(DynamicTraitedSpec):
base_dir = traits.Directory(exists=True,
Copy link
Member

Choose a reason for hiding this comment

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

Directory is defined in interfaces.traits_extension.py. Import from interfaces.base.

if self.inputs.raise_on_empty:
raise IOError(msg)
else:
warn(msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would advocate here for using nipype's logging:

At the top:

from .. import logging
(...)

LOGGER = logging.getLogger('workflows')

And here:

LOGGER.warning('Output key: %s returned no files', key)

mandatory=True)
output_query = traits.Dict(key_trait=Str,
value_trait=traits.Dict,
desc='Queries for outfield outputs')
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should give this field a more sensible default - for example {"func": {"modality": "func"}, "anat": {"modality": "anat"}}. This would save time figuring out the syntax for 80% of users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a fan. The current default of outputting all items is probably almost never usable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an other idea for sensible defaults: If no infields are defined, infields for all BIDS entities in project are automatically created, and Undefined by default. If user connects an input to them, great, if not, they are not passed to pybids. It saves having to define infields.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a great idea!

@chrisgorgo
Copy link
Member

Looks great - thanks for putting this together Alejandro.

@adelavega
Copy link
Contributor Author

I'm having trouble getting the tests to run because pybids's test data is not getting copied over.

In travis (for example), NIPYPE_EXTRAS are installed using -e, like this:

travis_retry pip install -e .[$NIPYPE_EXTRAS]

Installing pybids with -e should make the data folder available (which is not installed normally as its not a "package", on purpose). However, it looks like -e is supposed to be used with local files or VCS urls, and when used with pypi it just seems to do a normal install, without coping over the data.

Any clues on how to get pip to download the whole package and then install in editable mode, so that non-package folders are available?

@oesteban
Copy link
Contributor

oesteban commented Sep 13, 2017

Hi @adelavega, that way you are installing nipype in editable mode, but NOT pybids. To do so, just add some lines after this

rm -r ${CONDA_HOME}/lib/python${TRAVIS_PYTHON_VERSION}/site-packages/nipype*; }

rm -r ${CONDA_HOME}/lib/python${TRAVIS_PYTHON_VERSION}/site-packages/nipype*;
pushd $HOME;
git clone https://github.com/INCF/pybids.git;
cd pybids;
pip install -e .;
popd; }

It will get installed then in editable mode. Please note the closing bracket that originally was at line 39.

@adelavega
Copy link
Contributor Author

Sweet thanks. I was trying to avoid installing directly from github but if you don't mind, then that works for me!

@adelavega
Copy link
Contributor Author

adelavega commented Sep 14, 2017

Allright, I've incorporated all of your changes and got Travis CI working with the new tests. Any more feedback on "smarter" defaults is appreciated, and any hints on where to inject pybids pip -e installation into Circle.

Looks like it's failing because I pip install in the pre section, but It's trying to import elsewhere, perhaps in the docker images? Would it be a bad idea to the do the pip install in the test script itself?

@adelavega
Copy link
Contributor Author

Note: I changed the name of the file to bids_utils because otherwise python 2.7 imported that file instead of pybids in import bids

@satra
Copy link
Member

satra commented Sep 15, 2017

@adelavega - let's make sure we remove any commit in this tree that adds data to the repo.

@adelavega
Copy link
Contributor Author

Okay @satra I re-wrote history.

@adelavega
Copy link
Contributor Author

Yay, all tests passing on Circle and Travis. @satra, feel free to make a final review whenever you're ready.

@oesteban
Copy link
Contributor

Sorry @adelavega, yes, that's the right place to insert the installation of pybids 👍


# If infields is None, use all BIDS entities
if infields is None:
bids_config = join(dirname(gb.__file__), 'config', 'bids.json')
Copy link
Member

Choose a reason for hiding this comment

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

as a future improvement to a GrabbitIT interface that could serve as a base class, could this be passed along as a parameter?

this would also allow arbitrary and newer layouts to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The issue here is that I cannot get the entities of a project (or BIDS entities in general), without initializing a BIDSLayout with a specific dataset. So this was the only way I could think of to access the entities definition.

@satra
Copy link
Member

satra commented Sep 22, 2017

perhaps this should be added to io as a reference, since people are used to pulling out any io related things. it can stay in this file.

from nipype.interfaces.io import BIDS...

if you are up for reorganizing io into a directory with a base class and separate files for the different pieces. let's at least do the io reference for this one and then re-organize in a separate PR.

otherwise this LGTM

@adelavega
Copy link
Contributor Author

Great! I've added the reference @satra so feel free to merge.

@oesteban
Copy link
Contributor

Tests will require #2171 to be merged in first.

@satra
Copy link
Member

satra commented Sep 24, 2017

@adelavega - i think this is good to go, but just to be sure could you please merge with master? we had to fix a few things.

@adelavega
Copy link
Contributor Author

@satra done! travis passed so i assume circle will too soon

@satra satra merged commit 2e2884f into nipy:master Sep 25, 2017
@adelavega adelavega deleted the bids_grabber branch September 25, 2017 03:39
@satra satra added this to the 0.14.0 milestone Oct 20, 2017
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

7 participants