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] Initial adoption of templateflow #1334

Closed
wants to merge 31 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@oesteban
Copy link
Contributor

oesteban commented Oct 19, 2018

Changes proposed in this pull request

  • Implement new interface of templates in niworkflows-0.5.0, getting ready for templateflow
  • Deprecate --template and --output-spaces in favor of the new --output-references

Documentation that should be reviewed

  • Make sure that the sphinx extension generates a nice description of the new command line argument.
  • Workflows have been updated to render with the new workflow creation signatures.
help="""\
volume and surface spaces to resample functional series into
- T1w: subject anatomical volume
- MNI: spatial normalization using the target MNI template specified by --template

This comment has been minimized.

@effigies

effigies Oct 19, 2018

Collaborator

Could you talk about this decision to change template to MNI? Are we refusing to support templates in non-MNI space (except FreeSurfer)? How will this affect attempts to support developmental and/or non-human populations? If this is a move to be more in line with derivatives discussion of templates/spaces should we be changing the FreeSurfer options to --output-space FS and use --template fsaverage{,5,6}?

This comment has been minimized.

@oesteban

oesteban Oct 19, 2018

Author Contributor

Yes, I'm inclined to think that --output-space and --template will be merged in one argument (something like --output-reference) that will take values like orig, T1w, MNI152{Lin,NlinAsym2009c}, fsaverage{,5,6} and probably custom so then the template may be passed by path.

In the contrary to my views in the realm of BEP014 as regards space/coordinates/templates definitions, I believe that here we must be explicit about the reference (i.e. prefer just MNI<mni-id> over the combination of --output-space MNI --template MNI<mni-id>). However this seemed like a larger change that will also require arranging more templates in templateflow (and also the archive to keep it in sync since we are not yet accessing the templates through datalad).

This comment has been minimized.

@oesteban

oesteban Oct 19, 2018

Author Contributor

@effigies should we go for it in this PR? Do you even like the idea? WDYT?

@oesteban oesteban force-pushed the enh/templateflow branch from 9d043bb to e4023a9 Oct 26, 2018

@oesteban

This comment has been minimized.

Copy link
Contributor Author

oesteban commented Oct 26, 2018

This is ready for review. When niworkflows 0.5.0 is released, I'll pin it and this can be merged.

@oesteban

This comment has been minimized.

Copy link
Contributor Author

oesteban commented Oct 27, 2018

Concerns against merging this?

@oesteban

This comment has been minimized.

Copy link
Contributor Author

oesteban commented Oct 29, 2018

The merging of --output-space and --template into --output-references might be contended. Any voice against this PR? @effigies @chrisfilo @emdupre @jdkent

@effigies

This comment has been minimized.

Copy link
Collaborator

effigies commented Oct 29, 2018

Ah sorry. There are more comments on my earlier question than I realized.

Part of the reason I haven't commented more (apart from that) is that I'm having trouble thinking clearly about the end goal of what we want space/template specification to be. I think if we're going to change the CLI, we should be fairly confident that we'll be sticking with those options for a while. So my questions are really:

  1. Is this where we want to be long term? Will solidifying BEP 014 change anything about this? Your earlier comment seems to indicate that this is actually counter to how BEP 014 discusses things, so are we sure that the divergence will be less confusing than congruence?

  2. Is this necessary? Does failing to change this cause much code complication, or lead to unintuitive behavior for the user? My impression is that it's mostly a renaming, and it's not at all clear that the new names will gain as much in clarity as we'll lose in causing users to change their existing submission scripts.

Just as a reminder for any other participants, the reason we pulled --template out from --output-space was to limit the amount people would need to type a full template name. I see that you've made MNI a shorthand, so perhaps that's okay, but it doesn't seem intuitive that MNI means, very specifically, MNI152NLin2009cAsym. If anybody has been using an alternative --template, they're going to need to rewrite their scripts, although they'll also be the most likely to have thought a fair bit about these options and won't have too much trouble.

@jdkent

This comment has been minimized.

Copy link
Contributor

jdkent commented Oct 30, 2018

  1. Is this where we want to be long term? Will solidifying BEP 014 change anything about this? Your earlier comment seems to indicate that this is actually counter to how BEP 014 discusses things, so are we sure that the divergence will be less confusing than congruence?

@effigies could you give me more detail about point 1? Is the potential concern that --output-references is conflating coordinate systems (e.g. MNI, FS, etc) with spaces (e.g. MNI152, fsaverage)? Would the potential use case be someone that wants to use the FS coordinate system with MNI152 space? (<- is that easily achievable?)

  1. Is this necessary? Does failing to change this cause much code complication, or lead to unintuitive behavior for the user? My impression is that it's mostly a renaming, and it's not at all clear that the new names will gain as much in clarity as we'll lose in causing users to change their existing submission scripts.

I personally find it more clear to have --output-references as opposed to --template and --output-space. My (unpopular?) preference for submission scripts like this is to be as explicit as possible what the options are doing, so I prefer having the entire template name, it makes it easier for someone else to get the gist of what's going on (and me 6 months later).

@oesteban

This comment has been minimized.

Copy link
Contributor Author

oesteban commented Oct 30, 2018

Is this where we want to be long term? Will solidifying BEP 014 change anything about this? Your earlier comment seems to indicate that this is actually counter to how BEP 014 discusses things, so are we sure that the divergence will be less confusing than congruence?

I don't think this would be against BEP 014. On the contrary, the metadata of the transform files will need to include a key indicating the image that was used as reference in registration. So, changing the name from space to reference will be more aligned with BEP 014.

Is this necessary? Does failing to change this cause much code complication, or lead to unintuitive behavior for the user? My impression is that it's mostly a renaming, and it's not at all clear that the new names will gain as much in clarity as we'll lose in causing users to change their existing submission scripts.

I agree this is mostly a renaming, however it makes conceptually easier the implementation of the various behaviors we want to see. Additionally, the PR will not remove the old command line arguments as they will go through a deprecation cycle that we can make as long as we consider necessary.

I am a bit undecided about --template though. I think it is not particularly useful right now (you may pass in a path to a template file, but I don't think we have completely implemented the use case). But the new --output-references misses a language to tell pure-references and templates apart. It would not be too hard to implement (basically, only T1w, orig/boldref are just resampling references and any other keyword or path to folder containing a template are templates, ie. run spatial normalization to it).

@effigies

This comment has been minimized.

Copy link
Collaborator

effigies commented Oct 30, 2018

Okay. I'm not really trying to argue this out. I don't really understand how spaces and references and coordinate systems are supposed to work, but I'll assume you do.

@jdkent

This comment has been minimized.

Copy link
Contributor

jdkent commented Oct 31, 2018

I honestly don't know how much I understand about it either.

@emdupre

This comment has been minimized.

Copy link
Collaborator

emdupre commented Oct 31, 2018

I also will defer to your judgement @oesteban -- both for this CLI and for its alignment with BEP014 !

I did want to add, though, the fact that this is confusing to other maintainers does raise a bit of a concern for me. Could we add any additional documentation here, outside of just the CLI rendering ? I'm thinking in particular that it could go in the processing pipeline details.

@effigies

This comment has been minimized.

Copy link
Collaborator

effigies commented Oct 31, 2018

@oesteban Are you okay with pushing this off until the next release? I'd like to get a release ASAP so we can start producing RC-conformant derivatives.

@oesteban

This comment has been minimized.

Copy link
Contributor Author

oesteban commented Oct 31, 2018

Yep, let's cut a new release.

@effigies

This comment has been minimized.

Copy link
Collaborator

effigies commented Nov 9, 2018

Large chunks of this were incorporated into #1373. Please merge master before continuing.

effigies and others added some commits Nov 9, 2018

@oesteban

This comment has been minimized.

Copy link
Contributor Author

oesteban commented Jan 25, 2019

This PR has gone stale. Most of the functionality will be covered by the sMRIPrep merge, and the only open issue would be the new command line interface and how to handle output spaces.

I'll submit a PR with a new proposal when sMRIPrep is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.