8000 [MAINT] ensure transform on 3D volume (1D surface) images returns 1D arrays by Remi-Gau · Pull Request #5381 · nilearn/nilearn · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[MAINT] ensure transform on 3D volume (1D surface) images returns 1D arrays #5381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 45 commits into from
May 21, 2025
Merged

[MAINT] ensure transform on 3D volume (1D surface) images returns 1D arrays #5381

merged 45 commits into from
May 21, 2025

Conversation

Remi-Gau
Copy link
Collaborator
@Remi-Gau Remi-Gau commented May 15, 2025 •
edited
Loading

Changes proposed in this pull request:

Ensure the following

for nifti maskers

  • transform of 3D images returns 1D arrays
  • transform of 4D images returns 2D arrays
  • inverse_transform of 1D arrays return 3D images
  • inverse_transform of 2D arrays return 4D images
  • check_arrays before inverse transform

for surface maskers

  • transform of 1D images returns 1D arrays
  • transform of 2D images returns 2D arrays
  • inverse_transform of 1D arrays return 1D images
  • inverse_transform of 2D arrays return 2D images
  • check_arrays before inverse transform

Move and refactor tests and integrate them in estimat_check

TODO:

  • check that transform of 1D surface image return 1D array
  • check that inverse transform of 1D array return 1D surface image

@github-actions GitHub Actions
< 8000 /span> Copy link
Contributor

👋 @Remi-Gau Thanks for creating a PR!

Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft.

Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.

  • PR has an interpretable title.
  • PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
  • Code is PEP8-compliant (see our documentation on coding style)
  • Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

  • There is at least one unit test per new function / class (see our documentation on testing)
  • The new feature is demoed in at least one relevant example.

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

Remi-Gau
Remi-Gau commented May 15, 2025
View reviewed changes
@Remi-Gau Remi-Gau changed the title [MAINT] ensure transform on 3D volume images (and 1D surface image) returns 1D arrays [MAINT] ensure transform on 3D volume images returns 1D arrays May 15, 2025
@Remi-Gau Remi-Gau mentioned this pull request May 19, 2025
Merged
1 task
@codecov Codecov
Copy link
codecov bot commented May 19, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 99.39394% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.83%. Comparing base (320fbf4) to head (60685d8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
nilearn/_utils/estimator_checks.py 98.80% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5381   +/-   ##
=======================================
  Coverage   96.83%   96.83%           
=======================================
  Files         282      282           
  Lines       38992    38901   -91     
  Branches     3865     3885   +20     
=======================================
- Hits        37757    37671   -86     
+ Misses        680      674    -6     
- Partials      555      556    +1
Flag Coverage Δ
macos-latest_3.10_plotting 95.53% <99.39%> (+<0.01%) ⬆️
macos-latest_3.11_plotting 95.53% <99.39%> (+<0.01%) ⬆️
macos-latest_3.12_plotting 95.53% <99.39%> (+<0.01%) ⬆️
macos-latest_3.13_plotting 95.53% <99.39%> (+<0.01%) ⬆️
macos-latest_3.9_min 77.59% <99.39%> (-0.04%) ⬇️
macos-latest_3.9_plot_min 95.13% <99.39%> (+<0.01%) ⬆️
macos-latest_3.9_plotting 95.53% <99.39%> (+<0.01%) ⬆️
ubuntu-latest_3.10_plotting 95.56% <99.39%> (+<0.01%) ⬆️
ubuntu-latest_3.11_plotting 95.56% <99.39%> (+<0.01%) ⬆️
ubuntu-latest_3.12_plotting 95.56% <99.39%> (+<0.01%) ⬆️
ubuntu-latest_3.13_plotting 95.56% <99.39%> (+<0.01%) ⬆️
ubuntu-latest_3.13_pre 95.56% <99.39%> (+<0.01%) ⬆️
ubuntu-latest_3.9_plotting 95.55% <99.39%> (+<0.01%) ⬆️
windows-latest_3.10_plotting 95.53% <99.39%> (+16.55%) ⬆️
windows-latest_3.11_plotting 95.53% <99.39%> (+5.84%) ⬆️
windows-latest_3.12_plotting 95.53% <99.39%> (+5.77%) ⬆️
windows-latest_3.13_plotting 95.53% <99.39%> (+0.14%) ⬆️
windows-latest_3.9_plotting 95.53% <99.39%> (+6.98%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Remi-Gau Remi-Gau changed the title [MAINT] ensure transform on 3D volume (1D sufrace) images returns 1D arrays [MAINT] ensure transform on 3D volume (1D surface) images returns 1D arrays May 20, 2025
@Remi-Gau Remi-Gau added this to the 0.12.0 milestone May 20, 2025
@Remi-Gau Remi-Gau requested review from bthirion, man-shu and hndgzkn and removed request for bthirion and man-shu May 20, 2025 18:43
@Remi-Gau Remi-Gau marked this pull request as ready for review May 20, 2025 18:43
bthirion
bthirion reviewed May 20, 2025
View reviewed changes
Copy link
Member
@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

To me this is OK, except for one docstring.
thx !

Remi-Gau
Remi-Gau commented May 21, 2025
View reviewed changes
Remi-Gau
Remi-Gau commented May 21, 2025
View reviewed changes
man-shu
man-shu approved these changes May 21, 2025
View reviewed changes
Copy link
Contributor
@man-shu man-shu left a comment

Choose a reason for hiding this comment

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

LGTM!

@Remi-Gau
Copy link
A3D4
Collaborator Author

OK I will wait till CI is done and will stop changing it before this baby gets even bigger

@Remi-Gau Remi-Gau merged commit 8497b28 into nilearn:main May 21, 2025
35 checks passed
@Remi-Gau Remi-Gau deleted the 3D1D branch May 21, 2025 09:47
hndgzkn
hndgzkn approved these changes May 21, 2025
View reviewed changes
Copy link
Collaborator
@hndgzkn hndgzkn left a comment

Choose a reason for hiding this comment

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

Ok with very minor suggestions.

doc/manipulating_images/masker_objects.rst
Surface masker objects can transform both 1D, 2D,
as well as list of 1D or 2D surface image objects.
Transforming a 1D image produces a 1D (features,) array.
All other input will produce a 1D (samples, features) array..
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
All other input will produce a 1D (samples, features) array..
All other input will produce a 1D (samples, features) array.

doc/manipulating_images/masker_objects.rst
but to extract data from :class:`~nilearn.surface.SurfaceImage`.

They can perform data extraction from 1D surface data (n_vertices),
2D surface data (n_vertices x samples)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
2D surface data (n_vertices x samples)
2D surface data (n_vertices, samples)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently I am late :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops sorry

I need to do another pass on this so I can integrate those suggestions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bthirion bthirion bthirion left review comments

@hndgzkn hndgzkn hndgzkn approved these changes

@man-shu man-shu man-shu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Remi-Gau @bthirion @hndgzkn @man-shu
0