MNT: Increase minimum tedana version

[tsalo]

There is a new tedana release (0.0.9), in which the masking we apply is made less aggressive. This should address user-reported issues with multi-echo coverage in fMRIPrep (e.g., here, here, and ME-ICA/tedana#600).

Pinging @emdupre and @jbteves because they were involved in the release.

Changes proposed in this pull request

• Increase the minimum tedana version to 0.0.9, now that there is a new release.

Documentation that should be reviewed

None

[effigies]

@tsalo A couple points:

1. Can you merge master?
2. Can you explain what tedana needs scipy 1.3.3+ for? Are there bugs we should be expecting to see using tedana 0.0.9a1 that would be fixed by upgrading scipy? Also scikit-learn. (Looking at https://github.com/ME-ICA/tedana/pull/575/files#diff-9184081828959d098f4c5ff1be1111dbc955d9284508838db2182fd6863af7aa.)

[tsalo]

1. Can you merge master?

Can do.

EDIT: It looks like this branch is already up-to-date w.r.t. master.

2. Can you explain what tedana needs scipy 1.3.3+ for? Are there bugs we should be expecting to see using tedana 0.0.9a1 that would be fixed by upgrading scipy? Also scikit-learn. (Looking at https://github.com/ME-ICA/tedana/pull/575/files#diff-9184081828959d098f4c5ff1be1111dbc955d9284508838db2182fd6863af7aa.)

We updated the minimum requirement in ME-ICA/tedana#518 to fix test failures, but we never successfully pinned down the cause of the failing tests, and we aren't sure if 1.3.3 is necessary to fix things.

@emdupre @jbteves what do you think is the best solution? Should we test out the scipy version fMRIPrep has pinned in Dockerfile (1.1.0) with tedana to see if it breaks things and, if not, make our next release with that version in our requirements? Then I could change this PR to pin to 0.0.10 instead of 0.0.9.

[emdupre]

@emdupre @jbteves what do you think is the best solution? Should we test out the scipy version fMRIPrep has pinned in Dockerfile (1.1.0) with tedana to see if it breaks things and, if not, make our next release with that version in our requirements? Then I could change this PR to pin to 0.0.10 instead of 0.0.9.

I can open a PR on tedana now and see if it passes our test suite as-is ! We can re-visit once we have those results.

[effigies]

@tsalo @emdupre I see that ME-ICA/tedana#723 went in, so it looks like master will at some point ask for tedana ~= 0.0.10. What are your thoughts on hard-pinning 0.0.9a1 for the LTS (#2403), or should I go with tedana >=0.0.9a1,<0.0.11,!=0.0.9?

[tsalo]

To be honest, I'm not sure what changes are acceptable under the LTS version, but if it is acceptable to update tedana when that will change the results somewhat (combined data will be masked less aggressively), then I think that would be preferable for most users. If not, though, then your solution to pin 0.0.10 in master and 0.0.9a1 in 20.2.1 sounds good.

[effigies]

It comes down to whether you consider the change in masking a bug fix or a substantive change in processing. If it turns data from unusable to usable, then I'd consider it a bug fix. If, for data that was already masked sensibly, the change in masks is beyond the run-to-run variability, then I would err on the side of it not being a bug fix.

In cases where it's less clear, the LTS maintainer (in this case @bpinsard) is the final authority.

[bpinsard]

I agree with @effigies that it depends on whether you consider it a bugfix or not. I am not familiar enough with the multi-echo branch of the pipeline to make a decision right now.

[emdupre]

It comes down to whether you consider the change in masking a bug fix or a substantive change in processing. If it turns data from unusable to usable, then I'd consider it a bug fix. If, for data that was already masked sensibly, the change in masks is beyond the run-to-run variability, then I would err on the side of it not being a bug fix.

Thanks all ! To weigh in here: it's not that previous versions gave incorrect results, but those results may not have aligned with user expectations based on how this method was previously described in the literature. Essentially: While the previous code correctly generates T2* maps, it only does so in voxels where multiple echos have signal. Since multi-echo acquisitions are often described as increasing signal in regions which typically suffer from dropout, users were surprised that this was not the case in their processed data (because these dropout-prone regions typically only have signal in the first echo and so were not included in the generated T2* maps before this release).

So, I'd say it's really a judgment call. The maps from either tedana release are technically correct, but the maps from the more recent release will better match most user expectations. I can of course go into more detail on why we switched strategies for this t2smap workflow, if that helps in making this decision !

[effigies]

Thanks for the context, Elizabeth. Sounds like the more conservative choice is to consider it an improvement rather than a bug fix, but there's an argument here that users find the current results less usable.

If it were me, I'd probably go the conservative route, but I'll defer to Basille and Pierre if they think it makes sense to include this update.

[effigies]

@oesteban I think the goal was to move to 0.10+ once that was released.

[oesteban]

Ugh, thanks for the heads-up. I guess merging this can't harm.