Skip to content
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.

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

[ENH] add support for fitting first levels to numpy arrays directly #4112

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

henrymj
Copy link

@henrymj henrymj commented Nov 16, 2023

Changes proposed in this pull request:

  • adds function fit_arrays to FirstLevelModel; same as fit, but expects numpy arrays rather than niimgs
  • adds format_type argument to FirstLevelModel.compute_contrast. If array, it returns contrast results as arrays, rather than images

TODO

  • add tests (probably don't need to replicate all of the high-level calls to .fit ... maybe just simulate fake data and see if the results are the same?)
  • add example? (I know there isn't a plan to advertise this functionality)

Copy link
Contributor

👋 @henrymj 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.

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 78 lines in your changes are missing coverage. Please review.

Comparison is base (b8053f1) 91.69% compared to head (cd799bb) 91.25%.
Report is 82 commits behind head on main.

Files Patch % Lines
nilearn/glm/first_level/first_level.py 7.14% 76 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4112      +/-   ##
==========================================
- Coverage   91.69%   91.25%   -0.44%     
==========================================
  Files         144      144              
  Lines       16150    16231      +81     
  Branches     3359     3385      +26     
==========================================
+ Hits        14809    14812       +3     
- Misses        796      872      +76     
- Partials      545      547       +2     
Flag Coverage Δ
macos-latest_3.10 ?
macos-latest_3.11 ?
macos-latest_3.12 ?
macos-latest_3.9 ?
ubuntu-latest_3.10 ?
ubuntu-latest_3.11 91.17% <7.14%> (-0.44%) ⬇️
ubuntu-latest_3.12 ?
ubuntu-latest_3.9 91.13% <7.14%> (-0.44%) ⬇️
windows-latest_3.10 ?
windows-latest_3.11 ?
windows-latest_3.12 ?
windows-latest_3.9 ?

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.

@ymzayek
Copy link
Member

ymzayek commented Nov 17, 2023

The img input to fit need to be niimg-like objects to be compatible with the nifti masker. The added code seems to leave masking out to support working with np arrays so this is changing the fit functionality. Wouldn't it be easiest to just add some lines to fit() to cast the numpy array into a niimg-like object?

@henrymj
Copy link
Author

henrymj commented Nov 17, 2023

I was following @bthirion 's suggestion of making a separate function, but that does sound like it could be easier. Especially if it means it updates self.masker such that compute_contrast and get_voxelwise_attribute would work without modification, and users could just call get_data on the returned object to get back to 2D arrays.

Please pardon my ignorance - how would you cast numpy arrays with an arbitrary length along the first dimension (an arbitrary number of ROIs or edges being fit) into a niimg-like object?

@bthirion
Copy link
Member

I find it weird to transform arrays into a Nfiti1Image, from which we will extract the data array... It turns out that the GLM does not use any image information than the data array. So I find it better that way. But I'm open to other opinions !

@ymzayek
Copy link
Member

ymzayek commented Nov 20, 2023

I find it weird to transform arrays into a Nfiti1Image, from which we will extract the data array... It turns out that the GLM does not use any image information than the data array. So I find it better that way. But I'm open to other opinions !

@bthirion yes but the fit method in FirstLevelModel has built in functionality of masking the data to extract the data array before fitting the glm and this is the blocker I see since the maskers fit and transform methods expect a Nfiti1Image. Do you suggest a new fitting function that does not include this functionality? If so, we need to make this explicit

@bthirion
Copy link
Member

I find it weird to transform arrays into a Nfiti1Image, from which we will extract the data array... It turns out that the GLM does not use any image information than the data array. So I find it better that way. But I'm open to other opinions !

@bthirion yes but the fit method in FirstLevelModel has built in functionality of masking the data to extract the data array before fitting the glm and this is the blocker I see since the maskers fit and transform methods expect a Nfiti1Image. Do you suggest a new fitting function that does not include this functionality? If so, we need to make this explicit

Yes, that's the idea.

@ymzayek
Copy link
Member

ymzayek commented Nov 21, 2023

Ok I see thanks for clarifying. Then let's go ahead with the original plan @henrymj

@Remi-Gau Remi-Gau added the GLM Issues/PRs related to the nilearn.glm module. label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GLM Issues/PRs related to the nilearn.glm module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] support fitting first levels to numpy arrays directly, in addition to niimgs
4 participants