search

ME-ICA / tedana

TE-dependent analysis of multi-echo fMRI
https://tedana.readthedocs.io
GNU Lesser General Public License v2.1
161 stars 95 forks source link

Figures fail when TR is missing from header #332

Closed dowdlelt closed 5 years ago

dowdlelt commented 5 years ago

Summary

If the TR is missing from the dataset header, the --png figure creation process will fail. This is because the sampling rate is needed for the FFT - and a divide by zero error will occur.

Additional Detail

A user ran into this problem here: https://neurostars.org/t/tedana-denoising-package-axis-limit-issue-with-the-new-plotting-feature/4424 And it looks to be caused by using SPM to process the data, but it is unclear what specific step led to the problem. User reports that the BIDS validator also complained.

Next Steps

handwerkerd commented 5 years ago

If this issue only arises for labeling the x axis for the time series and FFT, when the TR is missing, why not just set the x axis to (0, TR/8, TR/4, TR/2, etc) rather than Hz for the FFT and volume number rather than seconds for the time series. It's a bit annoying for the visualization, but it would be accurate, and would naturally alert the user that TR isn't set, without crashing the entire program.

I don't have a strong opinion on this, but if a user doesn't know their precise TR, I'd rather let the program give a clear warning, but run without crashing rather than forcing them to enter a possibly wrong TR.

dowdlelt commented 5 years ago

I'm also not strongly opinionated on the subject either. Having clear warnings, but not failure for a visualization issue does seem like the least disruptive approach.

Perhaps for the a possible component reclassification step used in the future the program would need to fail fast and early.

emdupre commented 5 years ago

I wonder if we couldn't run the TR check right at the start of the workflow with an if png is True clause. I'm thinking just above here:

https://github.com/ME-ICA/tedana/blob/2ae6c486ea8b8fa3497635bf997b0aff1abcabda/tedana/workflows/tedana.py#L314

This way it would fail early, but only if the user has called figure generation. We could remove that if clause later if we end up needing an accurate TR more generally. WDYT ?

emdupre commented 5 years ago

I will say that I'm against letting the user provide their TR to us as an argument -- I think it's too likely to encourage bad practices. If we get an odd TR back we could encourage them to use the BIDS validator in the error text, though !

jbteves commented 5 years ago

I'm amenable to making that change. In #333 I added it right where you specified. Yeah, now that I've thought about it for a while, I'm not sure that providing a TR makes sense from the following standpoints:

  1. It's another CLI argument, and we have plenty of those.
  2. If the TR is weird, they should probably fix their file instead of coercing it into the state they want.

So @ME-ICA/tedana-devs I propose the following amendments to my PR:

  1. Remove --tr as an option.
  2. Check the TR only if the --png option is used.
  3. If the TR is incorrect, supply a fake TR of 1. Print the warning a second time at figure-generation.
  4. If the TR is 0 or 1, encourage them to use the BIDS validator or otherwise check

Then, if we eventually decide the TR is needed for some other calculation, all we have to do is drop the additional condition of the --png option being supplied.

Please comment below if you disagree; I'll start pushing changes tomorrow if not.

emdupre commented 5 years ago

Do you think it makes sense to give a fake TR, or just to fail with a descriptive error message ? I'm leaning towards the latter, personally, but willing to be convinced !

jbteves commented 5 years ago

I think that @handwerkerd is against failing with a bad TR for the whole pipeline. Personally, I'm pro-descriptive error message in the sense that it's really easy to code up and really obvious to the user what the problem is. On the other hand, I see his point and @dowdlelt 's that users might get frustrated if they kick off a bunch of runs and then come back to see that nothing actually happened. The warning and setting of a TR to 1 was my compromise approach. Re: making sense, I'm not sure that proceeding with a TR of 0 makes much sense to begin with.

handwerkerd commented 5 years ago

I'm perfectly fine with everything in @jbteves most recent proposal exactly defaulting to TR=1 if there isn't a valid TR, I'd much rather change the X axis label to "volumes" or "cycles/volume" over "sec" and "Hz." That would accurately describe the data. Also, defaulting to TR=1 in visualization means people who read the reports rather than the script outputs might not notice an issue.

emdupre commented 5 years ago

Also, defaulting to TR=1 in visualization means people who read the reports rather than the script outputs might not notice an issue.

I think this last point is the reason I'd rather fail fast if the user is requesting reports with no valid TR. It won't run any of the pipeline, just pop back with an error message.

rmarkello commented 5 years ago

If you do choose to come back with an error message instead of changing the x-axis label as @handwerkerd suggests it might be nice to also provide a link to somewhere showing them how to quickly modify the TR in the header, since that isn't immediately obvious to lots of people! You also probably don't want to assume they have neuroimaging-specific software installed (e.g., AFNI, FSL, whatever), so it might be good to show them how to do it using nibabel since tedana is pure Python for now.

jbteves commented 5 years ago

That's a good idea. Maybe we could use either a gist snippet or something like /docs/examples ?

dowdlelt commented 5 years ago

Seconding @rmarkello point - I'd have to google to know how to quickly change the header in a way that nibabel would like.

I love lots of command line arguments, but I buy the bad practices concern - so changing axis labels seems like a fix. However, I would only like that if it was left normal in the case where the TR is fine - I think giving people information that is immediately relevant to their task is highly valuable. As an example, I think the users being able to say 'oh hey this looks right, my task should start at 23 seconds and another thing at 65 and I see components matching that' is a really nice and compelling visual argument for the power of tedana.