Adding robustica option to ICA decomposition to achieve consistent results (new PR)

[handwerkerd]

#1013 was mis-aligned with main and there were some issues with getting it aligned again, so I'm opening a new PR that has multiple updates and should be aligned with main

Overview from that PR: Given that the FastICA method depends on its seed value, in some cases the generated output can be substantially different. To overcome this issue, we added robustica (https://github.com/CRG-CNAG/robustica/tree/main/robustica) which is a python library based on Icasso as an option to the ICA decomposition. robustica clusters many iterations of FastICA to achieve consistent results.

Changes proposed in this pull request:

• ica_method is added as an option (robustica or fastica) and the user can select between the two methods. The number of runs for when robustica is used can be determined by the user through setting n_robust_runs. The default is set to `robustica' with 30 number of robust runs. This default value is selected based on our experience using robustica for two different datasets with over 250 subjects' data.
• The default setting will continue to be fastica We should re-evaluate after more users start using robustica and provide feedback
• The DBSCAN clustering algorithm has been implemented in a hard-coded manner with Agglomerative clustering being used if that fails
• Explanations in the documentation are added to faq.rst and approach.rst

Note:

#1013 includes key discussions related to this PR including an evaluation of how the initial number of components requested affects the number of stable components found and an observation of similar ICA component spatial maps with different time series that shouldn't get lost with the switch to a new PR

[Lestropie]

Generating a task list to make sure nothing from #1013 gets missed:

• From this comment: Catching a failed RobustICA run is no longer a bare except, but it's catching all possible derivatives of Exception. @BahmanTahayori Do you have / can you generate a list of those exceptions raised by any of the invoked RobustICA functions, and then modify to only catch those?

• Current version of code adds "tedana/config.py". As per prior comment, "defaults.py" might be better for some (but is less appropriate for other constants currently in that file), or these constants could just be at the head of the main entrypoint source code file.

  • There was also the question of, if such an approach for defining constants is used, whether there is code unrelated to RobustICA that should be using the same approach to prevent unintended divergences. It looks like there's been some work in this respect; don't know if someone more familiar with the code base wants to review that it would now be used everywhere it's appropriate.

• Still awaiting from prior comment: Citation "Anglada2022" would be better renamed to "anglada2022robustica" (both removing capitalisation and adding first word of title) for consistency with the rest of the bibliography.

Looking through the code there's nothing setting off alarm bells to me, and the commit history looks appropriate.

[handwerkerd]

The tests are passing locally but failing here. The one thing I see that is clearly a problem is ERROR: Failed building wheel for scikit-learn-extra This is a library that is used within robustica. I just added it to pyproject.toml to see if that resolved the issue, but it did not. There are other errors, but I'm not sure if some/all are due to this problem. @tsalo, @eurunuela or others: Any ideas for how to get this built correctly on CircleCI?

[tsalo]

Could you pin the version?

Suggested change

	"robustica>=0.1.3, <=0.1.3",
	"robustica==0.1.3",

[handwerkerd]

Will this work with the version update bot? My assumption was we needed a <= to give the bot something to update, but I don't really know.

[tsalo]

dependabot should just bump the version. So if robustica releases 0.1.4, we'd get a PR changing it to robustica==0.1.4.

[handwerkerd]

Is that what we want or would "robustica>=0.1.3, <=0.1.4", be more in line with our other modules? I'm mostly neutral on this.

[eurunuela]

Hey @handwerkerd, I am getting the same error running the tests in a new environment. So I don't think it's a CircleCI issue.

[handwerkerd]

Hey @handwerkerd, I am getting the same error running the tests in a new environment. So I don't think it's a CircleCI issue.

Thanks! I think I found the root issue. When I created a clean environment, pip installed tedana and then tried to import robustica I see the following:

>>> from robustica import RobustICA

A module that was compiled using NumPy 1.x cannot be run in
NumPy 2.1.0 as it may crash. To support both 1.x and 2.x
versions of NumPy, modules must be compiled with NumPy 2.0.
Some module may need to rebuild instead e.g. with 'pybind11>=2.12'.

If you are a user of the module, the easiest solution will be to
downgrade to 'numpy<2' or try to upgrade the affected module.
We expect that some modules will need time to support NumPy 2.

numpy v2.0 was released in May 2024 and robustica had its last updated push to pypi (pip) in October 2023. My local python environment includes numpy<2.0 so this wasn't an issue. (As I sanity check, I'm now running our main with numpy=2.1 and it seems to be going well so this problem is limited to the new addition of robustica)

As for next steps, from my understanding, the most robust solution is to ask the makers of robustica push a new version to pipy that is compiled for numpy>=2.0 I'm opening an issue with them ( CRG-CNAG/robustica#6 ), but other ideas are also welcome. I'd rather not limit tedana to older versions of numpy unless that's absolutely necessary.

[Update: I just cloned robustica and tried a local install with pip install --use-pep517 -e . (also without the --upse-pep517` option). In both cases, I'm getting the same numpy message. I deleted all the pyc files and the environment included numpy=2.1 so I'm not sure how it could be compiled with numpy<2.0. I'm obviously missing something.]

[eurunuela]

I hope they are still around and can fix it, but the lack of commits and PRs since October last year doesn't sound very promising.

[handwerkerd]

I hope they are still around and can fix it, but the lack of commits and PRs since October last year doesn't sound very promising.

The author responded immediately, but I was looking around and it seems like one of their dependencies, scikit-learn-extra has the same problem. Unfortunately it sounds like that package is no longer actively maintained and a solution might take time: scikit-learn-contrib/scikit-learn-extra#177 If you look at that thread, there is a local-comple work-around, but they have to resolve some CI issues before they can release a new version on pipy

[BahmanTahayori]

Thanks @handwerkerd and the team. I will go through changes and will add a few other things as well. Hopefully, the issue with scikit-learn-extra can be resolved soon.

[handwerkerd]

I've been sitting on this for a few days, but I figured I should share. robustica is a single person's project. scikit-learn-extra is an unmaintained library with a few people keeping half-an-eye on it. i want this merged, but adding these as required dependencies might end up being more work for us longer term. We should discuss here or at our Sept 19th (20th in Australia) dev call.

The options I see are:

• If/when the current scikit-learn-extra issue is resolved, we can merge and hope things are stable for a while.
• Work with robustica to remove the scikit-learn-extra dependency (and we'd also understand robustica a bit better & potentially help with future issues there)
• Add a minimum version of robustica directly into our code (with appropriate credit)
• Use another method that we either add directly to our code, like we did for mapca, or is part of a package with more active developers.

Thoughts?

[BahmanTahayori]

Thanks @handwerkerd for your suggestions. Removing the scikit-learn-extra dependency or making it optional seems a reasonable choice. This library is used for two clustering methods KMedoids and CommonNNClustering. There are 10 other stable options from scikit-learn. Given that scikit-learn-extra is not maintained appropriately, I think we can negotiate with Miquel to see if he is happy to remove scikit-learn-extra. I will liaise with him.

[handwerkerd]

@BahmanTahayori fixed the original PR and has a potential solution to the scikit-learn-extra problem so I'm closing this PR and we can continue work in the original PR #1013