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: Make default native resolution explicit #494

Merged
merged 6 commits into from Apr 7, 2020

Conversation

mgxd
Copy link
Contributor

@mgxd mgxd commented Apr 3, 2020

Sets the default value of volumetric references resolution to native.

Originally discussed in #457 (comment), looks it is needed.

Addresses nipreps/fmriprep#2060 nipreps/fmriprep#2000

@pull-assistant
Copy link

@pull-assistant pull-assistant bot commented Apr 3, 2020

@mgxd mgxd requested a review from oesteban Apr 3, 2020
@codecov
Copy link

@codecov codecov bot commented Apr 3, 2020

Codecov Report

Merging #494 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
+ Coverage   62.86%   62.94%   +0.07%     
==========================================
  Files          42       42              
  Lines        5098     5098              
  Branches      742      740       -2     
==========================================
+ Hits         3205     3209       +4     
+ Misses       1739     1735       -4     
  Partials      154      154              
Flag Coverage Δ
#documentation 33.67% <0.00%> (-0.03%) ⬇️
#masks ?
#reportlettests 100.00% <ø> (ø)
#travis 57.89% <100.00%> (+0.03%) ⬆️
#unittests 45.78% <100.00%> (+0.04%) ⬆️
Impacted Files Coverage Δ
niworkflows/interfaces/bids.py 90.33% <100.00%> (+0.07%) ⬆️
niworkflows/utils/spaces.py 89.50% <100.00%> (+0.11%) ⬆️
niworkflows/interfaces/images.py 55.82% <0.00%> (+0.14%) ⬆️
niworkflows/viz/utils.py 80.06% <0.00%> (+0.24%) ⬆️
niworkflows/interfaces/mni.py 42.52% <0.00%> (+0.24%) ⬆️
niworkflows/utils/bids.py 77.17% <0.00%> (+0.82%) ⬆️

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 1977707...d830e55. Read the comment docs.

@mgxd
Copy link
Contributor Author

@mgxd mgxd commented Apr 3, 2020

With this change applied, the default BOLD outputs will now have the "res-native" identifier included in filenames.

I'm inclined to edit DerivativesDataSink to ignore this identifier, but open to any thoughts/concerns (cc @oesteban @effigies)

@mgxd mgxd force-pushed the fix/default-refs-native branch from bbcb1ca to be05b4e Compare Apr 6, 2020
@mgxd mgxd force-pushed the fix/default-refs-native branch from 8f7df56 to d830e55 Compare Apr 6, 2020
@effigies
Copy link
Member

@effigies effigies commented Apr 6, 2020

This looks fine, but I'm missing where it changes anything but filenames.

if (
val not in NONSTANDARD_REFERENCES
and not val.split(':')[0].startswith('fs')
and ":res-" not in val
and ":resolution-" not in val
):
# by default, explicitly set volumetric resolution to native
val = ":".join((val, "res-native"))
Copy link
Contributor Author

@mgxd mgxd Apr 6, 2020

Choose a reason for hiding this comment

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

@effigies res-native is already handled correctly, so this change just appends it to the output spaces by default if no resolution is specified

Copy link
Contributor

@oesteban oesteban left a comment

Sorry, I realize I have been pretty unclear on this one. When I said IMHO, spaces set via the --output-spaces option should always fill a res-native if no resolution modifier is specified I didn't mean that ReferenceSpace should have always a res-native. In other words, I would leave the responsibility of setting the res-native when resolution is missing to the cli module of fMRIPrep.

This solution has a lot of potential side-effects. Have you tested it locally?

@oesteban
Copy link
Contributor

@oesteban oesteban commented Apr 7, 2020

I'm inclined to edit DerivativesDataSink to ignore this identifier, but open to any thoughts/concerns

Check #334

@mgxd
Copy link
Contributor Author

@mgxd mgxd commented Apr 7, 2020

@oesteban actually this shouldn't have much side effects, since the only timeOutputReferencesAction is used is in the CLI for the user requested output spaces - the way we add internal spaces shouldn't be affected.

Check #334

that looks useful, but don't see how we can leverage it here - res-native isn't included in the source file, but rather in the provided space.

Copy link
Contributor

@oesteban oesteban left a comment

Okay, I see your points. I'm most concerned about whether we are already sticking the res- entity into the space input of the DataSinks. If so, then I think it is time to give them (the DataSinks) the love they deserve.

Also, I'm not convinced we should always drop the res-native bit. If there are other resolutions we might as well keep it. DYT?

This case is not similar to space, for which there is indeed one space matching the original image (and therefore, since there's no change on the property, it should not be added to the filename).

if self.inputs.space and '_res-native' in self.inputs.space:
# strip native tag
self.inputs.space = self.inputs.space.split('_')[0]

Copy link
Contributor

@oesteban oesteban Apr 7, 2020

Choose a reason for hiding this comment

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

Can't we just drop the resolution entity in the connection? Or the resolution entity is currently crammed into the space input in fMRIPrep and you are working around that hacky solution (honestly, I can't remember)?

Copy link
Contributor Author

@mgxd mgxd Apr 7, 2020

Choose a reason for hiding this comment

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

the resolution is appended to the space input, a hack we implemented to allow multiple resolutions of the same template

@@ -660,10 +660,14 @@ def __call__(self, parser, namespace, values, option_string=None):
spaces.checkpoint()
for val in values:
val = val.rstrip(":")
# Should we support some sort of explicit "default" resolution?
# https://github.com/nipreps/niworkflows/pull/457#discussion_r375510227
Copy link
Contributor

@oesteban oesteban Apr 7, 2020

Choose a reason for hiding this comment

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

Can we keep this reference and add a reference to this PR as comments?

Copy link
Contributor Author

@mgxd mgxd Apr 7, 2020

Choose a reason for hiding this comment

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

sure

@mgxd
Copy link
Contributor Author

@mgxd mgxd commented Apr 7, 2020

Also, I'm not convinced we should always drop the res-native bit. If there are other resolutions we might as well keep it. DYT?

I went back and forth here - ultimately I went with the route of least changes to the documentation. Do we want to make this explicit now (its been implicit for a long time) at the cost of renaming the expected output files?

If we have other resolutions, those others will have the res- fields included. It seems off to produce different filenames based on how many output spaces were requested

@oesteban
Copy link
Contributor

@oesteban oesteban commented Apr 7, 2020

I'm also bouncing back and forth on this one. WDYT @effigies ?

@effigies
Copy link
Member

@effigies effigies commented Apr 7, 2020

I would lean toward keeping it invisible.

res/den are only required to distinguish from another file. The goal of BIDS is not to put all relevant metadata in the file name but to help people quickly distinguish files. All other resolutions will have a res-* entity, so that seems fine.

@mgxd
Copy link
Contributor Author

@mgxd mgxd commented Apr 7, 2020

great, thanks for the reviews!

@mgxd mgxd merged commit d109e99 into nipreps:master Apr 7, 2020
@mgxd mgxd deleted the fix/default-refs-native branch Apr 7, 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.

None yet

3 participants