Skip to content

deal with nilearn changes#67

Merged
eurunuela merged 3 commits intoME-ICA:mainfrom
handwerkerd:nilearn-changes
Mar 20, 2026
Merged

deal with nilearn changes#67
eurunuela merged 3 commits intoME-ICA:mainfrom
handwerkerd:nilearn-changes

Conversation

@handwerkerd
Copy link
Copy Markdown
Member

niilearn moved some functions from nilearn._utils.niimg_conversions to nilearn.image. We added clause to deal with this in tedana, but this just caused an issue in mapca so I added it here as well.

An integration test is now failing. I briefly looked at the failure and it looks like values flipped postive/negative, but I haven't dug more yet.

@handwerkerd
Copy link
Copy Markdown
Member Author

Looks like the tests passed here, but not locally for me. Someone else should also try a local test while using nilearn v0.13.1.

@handwerkerd handwerkerd requested a review from eurunuela March 20, 2026 01:47
Copy link
Copy Markdown
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also get the same error locally. I wonder why it has the sign flipped now on 29 components but not on 28... shall we just go with the absolute value for the np.allclose()?

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates mapca to remain compatible with recent nilearn internal API changes by conditionally importing check_niimg_3d / check_niimg_4d from the new location when available.

Changes:

  • Add a nilearn version gate to import check_niimg_3d / check_niimg_4d from nilearn.image for nilearn>=0.13.0.
  • Fall back to the previous import path (nilearn._utils.niimg_conversions) for older nilearn versions.
  • Introduce packaging.version.Version usage to compare versions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mapca/mapca.py
Comment on lines +25 to +35
from nilearn import __version__ as nilearn_version
from nilearn import masking
from nilearn._utils.niimg_conversions import check_niimg_3d, check_niimg_4d
from packaging.version import Version
from scipy.stats import kurtosis
from sklearn.decomposition import PCA
from sklearn.preprocessing import StandardScaler

if Version(nilearn_version) >= Version("0.13.0"):
from nilearn.image import check_niimg_3d, check_niimg_4d
else:
from nilearn._utils.niimg_conversions import check_niimg_3d, check_niimg_4d
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packaging is used for Version(...) here, but it is not declared as a direct runtime dependency in pyproject.toml. This can cause an ImportError for users/environments where packaging is not installed. Consider either adding packaging to the project's dependencies, or avoid the extra dependency by using a try/except ImportError import from nilearn.image with a fallback to nilearn._utils.niimg_conversions (which also removes the need to compare versions).

Copilot uses AI. Check for mistakes.
@handwerkerd
Copy link
Copy Markdown
Member Author

handwerkerd commented Mar 20, 2026

I also get the same error locally. I wonder why it has the sign flipped now on 29 components but not on 28... shall we just go with the absolute value for the np.allclose()?

Do you want allclose or simply abs? If I'm reading correctly, allclose is the abs of the difference, but would give a large value if there's just a sign flip. I completely misread your comment. You are suggesting the same thing.

[Update]: I realized we don't want absolute value because we don't want any combination of positive or negative values, we want a consistent flip of the entire matrix. That said, the test is having trouble running locally inside my firewall right now, so also try locally.

@handwerkerd
Copy link
Copy Markdown
Member Author

I just ran locally (got an SSL issue fixed) and just pushed a fix that passed. It was a bit trickier because not all lines were flipped. I ended up taking the first value of each row and flipping if the signs of the two matrices were different.

Copy link
Copy Markdown
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass locally. I'm happy with the workaround for the signs.

Thank you @handwerkerd!

@eurunuela eurunuela merged commit 8b6e6f8 into ME-ICA:main Mar 20, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants