search

mne-tools / mne-bids-pipeline

Automatically process entire electrophysiological datasets using MNE-Python.
https://mne.tools/mne-bids-pipeline/
BSD 3-Clause "New" or "Revised" License
139 stars 66 forks source link

BUG: report can contain stale stats when one subject is used #900

Open SophieHerbst opened 6 months ago

SophieHerbst commented 6 months ago

I managed to fool myself by forgetting to remove the subject variable from my config after debugging, and looking at the sub-average report, which was done for only one subject. I think this could be prevented by printing the subject count in the report any time a figure is added, in particular for the decoding plots. For example: title = f"CSP TF decoding: {cond_1} vs. {cond_2}, N = {n_sbs}" I can give it a go if you agree.

hoechenberger commented 6 months ago

I agree 😀

larsoner commented 6 months ago

Agreed!

SophieHerbst commented 6 months ago

Looking into the code, the stats shouldn't have been run if there is only one subject:

Screenshot 2024-03-22 at 12 02 55
larsoner commented 6 months ago

Looking into the code, the stats shouldn't have been run if there is only one subject:

In this case we should maybe make sure to remove the section if it's present:

https://mne.tools/dev/generated/mne.Report.html#mne.Report.remove

To decide if we need to remove it or not we probably need https://github.com/mne-tools/mne-python/issues/12424

I'll reopen an re-title since it seems like there is an issue to be fixed still

larsoner commented 6 months ago

I realized this problem is more widespread than we thought. For example if you run once with decoding enabled in your config, then run again with it disabled (perhaps with some other changes like filtering or whatever), that original decoding report from potentially long ago when you ran with it enabled will still be in the report.

Same thing for stuff like autobad maxwell report stuff, etc. -- enable it then disable it and the info will still show up in the report. Worse there the TSV with the bads won't get overwritten so you'll be stuck with whatever autobads it found :fearful:

Same thing with if you switch between SSP and ICA I think (untested but I think it's the case).

So the safest thing I think is to never use the Skipping ... like we do now, and have a conditional inside the step itself that always either adds to the report section(s) or removes them (and maybe some output files in some cases?) depending on whether or not the step is used.

hoechenberger commented 6 months ago

Wow, omg…