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: Separate Space and SpatialReferences #1

Closed

Conversation

oesteban
Copy link

@oesteban oesteban commented Jan 24, 2020

Okay, after much thinking, I believe this captures the whole set of requirements.

Now there is an Space object that makes all the validations and sanity checks. There is still room for enhancements:

  • We might want to implement a classmethod (something like Space.from_string) that implements the parsing of --output-spaces arguments (see https://www.attrs.org/en/stable/init.html). DONE
  • We might want to check that the specs contain valid keywords against templateflow (at the moment, this is only done for cohort).

Other than that, I think the doctest examples show pretty much all the functionality.

Then, the SpatialReferences class is the one in charge of sorting out the available spaces. It has three convenience methods to filter out references:

  • get_std_spaces() returns template names (including cohort if necessary) that is of interest to the spatial normalizations that sMRIPrep will run.
  • get_templates() returns full Space objects with the desired outputs, as set by the user.
  • get_nonstd_spaces() returns nonstandard Spaces or their names (depending on the only_names argument)

@oesteban
Copy link
Author

Added tests on examples like those in: nipreps/fmriprep#1604 (comment)

@oesteban oesteban marked this pull request as ready for review January 24, 2020 02:43
@oesteban oesteban requested a review from mgxd January 24, 2020 02:43
@oesteban
Copy link
Author

Moving to niworkflows, since the changes are quite massive - I guess there's no point in looking at the diff.

@oesteban oesteban closed this Jan 24, 2020
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.

1 participant