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

[WIP] 0.7 release #247

Merged
merged 64 commits into from
Oct 6, 2018
Merged

[WIP] 0.7 release #247

merged 64 commits into from
Oct 6, 2018

Conversation

tyarkoni
Copy link
Collaborator

@tyarkoni tyarkoni commented Sep 12, 2018

This PR introduces major, API-breaking changes, and should probably prompt a minor version increase (to 0.7). Summary of changes:

  • Rename some entities to match the official BIDS-Raw spec more closely (notably, type becomes suffix and modality becomes datatype).
  • Changed BIDSLayout initialization API: paths becomes root, and must be a single (mandatory) string. A new derivatives argument specifies how to handle derivatives.
  • The module names are simplified: grabbids is now gone (in favor of layout) and bidslayout.py and bidsvalidator.py are just layout.py and validation.py. A deprecation warning is no longer issued (we had said this change would happen in 0.8, but I think it's better to introduce it early and have people fix all of the breaking changes at once).
  • Metadata in JSON sidecars is now indexed and searchable. Users can now pass arbitrary keys in as additional kwargs in BIDSLayout.get(); any arguments not recognized as entities are treated as metadata fields (closes Index/search metadata fields #243, closes Unified interface for searching over both filenames and metadata #254).
  • A new BIDSFile class wraps grabbit's File; this is a lightweight wrapper that adds .image and .metadata properties, which respectively return a NiBabel image object and a dictionary of metadata, respectively (closes Create BIDSFile object that provides easier access to metadata and image #242).
  • Built-in HRF convolution is now supported via bundling of nistats' hemodynamic_models module. The BIDS-Model spec will change to treat HRF convolution as a standard transformation; a convolve_HRF transformation has been added (see Add support for HRF convolution #250, Treat HRF convolution as a BIDS-Model transformation #251).
  • Derivatives and other non-core directories mentioned in the BIDS-Raw spec (code/, stimuli/, etc.) are no longer indexed by default, and must be explicitly included at initialization (via either the paths or include arguments). See Do not auto-index <bids-root>/derivatives/ #184.
  • The exclude argument has been removed from BIDSLayout, and the include argument now functions differently--it allows explicit inclusion of directories that would otherwise be dropped from indexing (see previous point).
  • File validation is now enabled by default (i.e., validate=True in BIDSLayout(); see Returning hidden files, names starting with dot #244). It also now works (see Layout initialization with validate=True fails #222). Note that I've temporarily bundled the regex rules from BIDS-validator-common with this package while we wait for them to be refactored into BIDS-validator. Eventually, we will install the latter as a dependency of pybids.
  • The auto_contrasts field in the modeling tools is now in compliance with the BIDS-Model spec (see analysis: default auto_contrasts behavior #234).
  • Directories marked as derivatives are now automatically also considered BIDS-Raw (Make bids config a dependency of derivatives #246).
  • Variable loading via load_variables can now be done incrementally, by repeatedly passing in a NodeIndex object in the dataset argument. This allows handling of edge cases where different sets of variables are tied to different sets of images (e.g., one can selectively load event files associated with image sets in different derivatives directories).
  • Expanded and improved path-building via layout.build_path(entities); it should now be possible to define a complete set of paths. The current set of paths is not quite complete, but includes most of the major file types, so this closes file writing functionality #63.
  • Various other minor bug fixes (e.g., get_collections fails when merge=True and the result is empty #202).

@tyarkoni
Copy link
Collaborator Author

'derivatives' now implies 'bids', so the fact that there are a lot of files is fine; it's just that probably the second specification is overwriting the first, which is not good.

@adelavega
Copy link
Collaborator

adelavega commented Sep 21, 2018

No, it's weird because the second directory is empty. It's just a dummy directory that also causes this problem.

Ah, but I see what you mean, that would explain the missing .json files

@effigies
Copy link
Collaborator

@tyarkoni Is there anything I can do to help this along? I'm not sure where to begin reviewing.

@tyarkoni
Copy link
Collaborator Author

Sorry, I've been stuck in grant and paper deadline hell. I think the only things that should be addressed before we can merge are (a) the bug Alejandro mentioned above (though it may predate the changes introduced here) and (b) making sure that fitlins still works after any tweaks necessary for 0.7 compatibility (e.g., being able to pass in convolutions as transformations instead of a separate section).

@tyarkoni
Copy link
Collaborator Author

Oh, and also, I think at this point we may as well wait for the first BIDS-Common-Derivatives RC, so that we can include all the new proposed entities. I'm 90% sure they won't require any breaking changes, but I'd rather be cautious about it--I think I'm probably already testing people's patience (and not just you ;)) by breaking the API so often.

@effigies
Copy link
Collaborator

What changes do I need to make to models for testing? Or should existing models still work?

@tyarkoni
Copy link
Collaborator Author

tyarkoni commented Sep 27, 2018

I think you just need to remove the HRF_variables field, and instead add a transformation called convolve_HRF, where the input is the list of variables you want to convolve. Otherwise I think existing models should work.

@tyarkoni tyarkoni mentioned this pull request Oct 4, 2018
@tyarkoni
Copy link
Collaborator Author

tyarkoni commented Oct 6, 2018

Okay I think I'm ready to merge this once you sign off, @adelavega and @effigies (no need to review everything, I just want to know that these changes don't break anything you've already written in a way that isn't relatively easily fixable).

@tyarkoni
Copy link
Collaborator Author

tyarkoni commented Oct 6, 2018

Actually, this PR is already gargantuan, so in the interest of keeping things tidy, I'm going to merge now (without yet bumping version).

@tyarkoni tyarkoni merged commit ce41dd0 into master Oct 6, 2018
@adelavega
Copy link
Collaborator

The fix seems good to me in principle. However, this line is problematic:

if not derivatives:

It seems to include bids_dir/derivatives even if derivatives has been set to another directory. I think this can be fixed by changing to if derivatives == True (and would be consistent with the docstring, which asks you to explicitly include derivatives if you are specifying bids_dir/derivatives as a string)

@tyarkoni
Copy link
Collaborator Author

tyarkoni commented Oct 7, 2018

@adelavega it's actually a real pain in the ass to handle this properly... the issue is what to do when, e.g., someone wants to index "/bids/derivatives/fmriprep", while ignoring everything else in "/bids/derivatives". Right now grabbit is set up to give precedence to exclude directives, which means the only way to get fmriprep to index, but ignore everything else, is to explicitly exclude everything else in the derivatives directory but not fmriprep. I was trying to finesse this and avoid having to rewrite grabbit, but I agree that probably won't fly.

@yarikoptic yarikoptic added this to the 0.7.0 release milestone Oct 11, 2018
@effigies effigies deleted the lets-break-stuff branch December 14, 2018 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants