-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat(get_trt): Allow estimated and fallback TotalReadoutTime #477
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #477 +/- ##
==========================================
- Coverage 83.86% 83.85% -0.01%
==========================================
Files 30 30
Lines 2807 2819 +12
Branches 360 365 +5
==========================================
+ Hits 2354 2364 +10
- Misses 383 384 +1
- Partials 70 71 +1 ☔ View full report in Codecov by Sentry. |
9585cbb
to
fcd3b8a
Compare
fcd3b8a
to
24332c1
Compare
@@ -37,6 +37,8 @@ | |||
class _GetReadoutTimeInputSpec(BaseInterfaceInputSpec): | |||
in_file = File(exists=True, desc="EPI image corresponding to the metadata") | |||
metadata = traits.Dict(mandatory=True, desc="metadata corresponding to the inputs") | |||
use_estimate = traits.Bool(False, desc='Use "Estimated*" fields to calculate TotalReadoutTime') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand Chris Rorden's thinking, this perhaps is best if enabled? Also, usedefault
is missing
use_estimate = traits.Bool(False, desc='Use "Estimated*" fields to calculate TotalReadoutTime') | |
use_estimate = traits.Bool(True, usedefault=True, desc='Use "Estimated*" fields to calculate TotalReadoutTime') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought was that since this isn't BIDS, using these fields should be opt-in instead of default.
Edit: Estimated*
adds some friction where the user needs to take affirmative action to use these estimates, so I see this approach as respecting that intention. I'm not wedded to this position, though, so I'm okay being overruled.
We previously allowed a KeyError to be raised, but that makes falling back more difficult. This puts the entire PEDir-dependent branch inside an if-block.
This is work toward nipreps/fmriprep#3009.
The primary purpose is to recover SyN-SDC in cases where TRT is not calculable. When we calculated displacement fields and applied them without round-tripping through a field in Hz, this didn't matter. Now that we do, it would be good to allow tools to supply a trivial value in these cases. I would suggest a power of 2 that is in the neighborhood of a plausible value, such as 0.03125, which only modifies the exponent of a floating point value, and not the significand, minimizing the potential for accumulating floating point error.
While I'm here, I provide a boolean flag (
False
by default) to fall back to EstimatedTotalReadoutTime or EstimatedEffectiveEchoSpacing.The order of precedence (if both
use_estimated
andfallback
are enabled) is:We could instead move
Estimated*
to afterWaterFatShift
, if that would be better.Closes #249.