FIX: Simplify STC logic for too short BOLD series #2489
Conversation
| return 1 | ||
|
|
||
| if skip_vols is None: | ||
| skip_vols = _get_vols_to_discard(img) |
There was a problem hiding this comment.
We lose the case where the series has >5 volumes but <5 taking into account dummy scans.
There was a problem hiding this comment.
Yes, this is what I meant (and definitely failed to convey) above.
|
After some thinking, I believe the problem we were trying to avert with the check removed by this PR should actually end up in an error. However, the specific error that My last commit 18f5e72 patches the interface so that the error is anticipated by the interface itself and a more understandable message is delivered to the user. CircleCI's build of ds210 should now crash with the new error (I'll fix that in an upcoming commit once this is confirmed to work properly). I'm aware of the cost of not calculating steady states at workflow building time is delaying the crash - but it is preferable that the user suffers an error down the line rather than just skipping STC and cross fingers that they will read the reports and the boilerplate, IMHO. |
|
Okay, the new edit seems to work well. Now ds210 is crashing at the STC node with: I'm going to switch this test to have |
After nipreps#2461 and nipreps#2468, the ``TooShort`` value for running STC could not be assigned. This PR ignores manually set dummy timepoints, although the case when this could be problematic is largely a very edge case (say you have only 7 timepoints and manually set 3 as dummy). On the flip side, this PR drops the use of a function discontinued in the latest niworkflows release, fixing a bug ahead of time.
Minimal stylistic changes and a better structure with a ToC in the beginning are sneaked into this commit - sorry about that.
oesteban
force-pushed
the
fix/deprecated-niworkflows-function
branch
from
faa845c
to
3e2d0d8
Compare
After #2461 and #2468, the
TooShortvalue for running STC could not be assigned.This PR ignores manually set dummy timepoints, although the case when this could be problematic is largely a very edge case (say you have only 7 timepoints and manually set 3 as dummy).
On the flip side, this PR drops the use of a function discontinued in the latest niworkflows release, fixing a bug ahead of time (currently popping out in #2392).