[Fix] Fix pathlib.Path not being counted as Niimg-like in new_image_like #3723

mcsitter · 2023-04-25T23:21:46Z

I received TypeError: The reference image should be a niimg when putting a pathlib.Path as an argument into nilearn.image.new_image_like. I fixed it to properly accept Niimg-like objects, which include pathlib.Path in its definition.

Changes proposed in this pull request:

Check for isinstance of pathlib.Path as well as str

github-actions · 2023-04-25T23:21:56Z

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

codecov · 2023-04-26T00:03:33Z

Codecov Report

Merging #3723 (a19134f) into main (6fe4c71) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3723   +/-   ##
=======================================
  Coverage   91.53%   91.53%           
=======================================
  Files         134      134           
  Lines       15751    15753    +2     
  Branches     3281     3281           
=======================================
+ Hits        14418    14420    +2     
  Misses        784      784           
  Partials      549      549

Flag	Coverage Δ
macos-latest_3.10	`91.53% <100.00%> (+<0.01%)`	⬆️
macos-latest_3.11	`91.53% <100.00%> (+<0.01%)`	⬆️
macos-latest_3.8	`?`
macos-latest_3.9	`91.49% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.10	`91.53% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.11	`91.53% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.8	`91.49% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.9	`91.49% <100.00%> (+<0.01%)`	⬆️
windows-latest_3.10	`91.47% <100.00%> (+<0.01%)`	⬆️
windows-latest_3.11	`91.47% <100.00%> (+<0.01%)`	⬆️
windows-latest_3.8	`91.43% <100.00%> (+<0.01%)`	⬆️
windows-latest_3.9	`91.43% <100.00%> (+<0.01%)`	⬆️

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

Files Changed	Coverage Δ
nilearn/image/image.py	`95.67% <100.00%> (+0.02%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

NicolasGensollen

Thanks for contributing @mcsitter
I might be missing something, but you should be able to provide a Path object to these functions. The path object is first casted to a string by stringify_path:

>>> stringify_path(Path("./foo.json"))
'foo.json'

nilearn/image/image.py

ymzayek · 2023-04-26T12:00:05Z

@mcsitter could you provide a reproducible example that will fail if we run it? As @NicolasGensollen pointed out, this case is supposed to be handled by stringify_path. Also can you check and report the version of nilearn in your environment? This would fail in nilearn versions less than 0.9.2.

mcsitter · 2023-04-27T20:02:00Z

@ymzayek I am on nilearn version 0.10.0. Here is some could which should allow you to reproduce the TypeError:

import pathlib

import nilearn.image
from nilearn.datasets import MNI152_FILE_PATH

image_path = pathlib.Path(MNI152_FILE_PATH)
image = nilearn.image.load_img(image_path)
nilearn.image.new_img_like(image_path, image.get_fdata())

ymzayek · 2023-04-28T07:51:00Z

@mcsitter ok I see, indeed new_img_like doesn't take a path-like object for the reference image. Though I suggest to use stringify_path function as a solution like the other functions in this module and then leave the rest of the code untouched

mcsitter · 2023-04-28T19:04:29Z

@ymzayek I changed the code to use stringify_path.

bthirion

Thx ! We should add the corresponding unit test to enforce this behavior. Can you do that ?

ymzayek · 2023-06-12T09:13:16Z

@mcsitter can you define a test under nilearn/image/tests/test_image.py ?
Also, you need to add your name in ~~doc/changes/names.rst~~ CITATION.cff and you'll need to rebase on main and resolve the conflicts. Let me know if you need any help.

ymzayek · 2023-08-02T09:26:15Z

@mcsitter are you able to pick this back up and finish it? Otherwise, I can take it over and make the necessary changes.

mcsitter · 2023-08-09T21:20:18Z

I am not sure I did the rebasing correctly... But I added a unit test, hopefully, a useful one, and added my name to the CITATION.cff.

Feel free to let me know if there is anything else I can do or how I would fix any mishaps regarding the rebasing.

Remi-Gau · 2023-08-09T22:35:12Z

Given the number of line and file changes I would say that something looks not right.

Am AFK at the moment but I will have a look when on a screen that is more than 20 characters wide.

ymzayek

@mcsitter I fixed the rebase and made a few comments to get the doc build to pass. This should be good to go now. @Remi-Gau can you also give a final review?

CITATION.cff

doc/changes/latest.rst

nilearn/image/tests/test_image.py

Remi-Gau

yup it looks good to me

ymzayek · 2023-08-10T11:41:06Z

Oops let me fix the flake8 error

Remi-Gau · 2023-08-10T11:54:43Z

nilearn/image/tests/test_image.py

+    data = np.random.rand(10, 10, 10)
+    img = Nifti1Image(data, np.eye(4))


not needed for now but I think there may be a lot random img creation in the whole test suite that could be fixturised

Linking #3725 and #3331

ymzayek

Thx! Merging.
Failure is unrelated (also failing on main)

NicolasGensollen reviewed Apr 26, 2023

View reviewed changes

nilearn/image/image.py Outdated Show resolved Hide resolved

bthirion reviewed May 8, 2023

View reviewed changes

mcsitter added 3 commits August 9, 2023 22:42

Fix pathlib.Path not being counted as Niimg-like

3f0d042

Fix unnecessarily changing mean_img

912f075

Change isinstance check to stringify_path

3bf4135

mcsitter added 2 commits August 10, 2023 11:58

Add unit test for acceptance of pathlib.Path as image

9b9b771

Add name to citation

49d044f

ymzayek force-pushed the main branch from e044a3a to 49d044f Compare August 10, 2023 10:05

ymzayek reviewed Aug 10, 2023

View reviewed changes

CITATION.cff Outdated Show resolved Hide resolved

doc/changes/latest.rst Outdated Show resolved Hide resolved

nilearn/image/tests/test_image.py Show resolved Hide resolved

ymzayek added 3 commits August 10, 2023 12:34

Update CITATION.cff

99840ee

Update doc/changes/latest.rst

6342121

Update nilearn/image/tests/test_image.py

77ed1a2

Remi-Gau approved these changes Aug 10, 2023

View reviewed changes

Fix flake8

a19134f

Remi-Gau reviewed Aug 10, 2023

View reviewed changes

ymzayek approved these changes Aug 10, 2023

View reviewed changes

ymzayek merged commit 438d243 into nilearn:main Aug 10, 2023
28 of 29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Fix pathlib.Path not being counted as Niimg-like in new_image_like #3723

[Fix] Fix pathlib.Path not being counted as Niimg-like in new_image_like #3723

mcsitter commented Apr 25, 2023

github-actions bot commented Apr 25, 2023 •

edited by Remi-Gau

codecov bot commented Apr 26, 2023 •

edited

NicolasGensollen left a comment

ymzayek commented Apr 26, 2023

mcsitter commented Apr 27, 2023

ymzayek commented Apr 28, 2023

mcsitter commented Apr 28, 2023

bthirion left a comment

ymzayek commented Jun 12, 2023 •

edited

ymzayek commented Aug 2, 2023

mcsitter commented Aug 9, 2023

Remi-Gau commented Aug 9, 2023

ymzayek left a comment

Remi-Gau left a comment

ymzayek commented Aug 10, 2023

Remi-Gau Aug 10, 2023

ymzayek Aug 10, 2023

ymzayek left a comment

		data = np.random.rand(10, 10, 10)
		img = Nifti1Image(data, np.eye(4))

[Fix] Fix pathlib.Path not being counted as Niimg-like in new_image_like #3723

[Fix] Fix pathlib.Path not being counted as Niimg-like in new_image_like #3723

Conversation

mcsitter commented Apr 25, 2023

github-actions bot commented Apr 25, 2023 • edited by Remi-Gau

codecov bot commented Apr 26, 2023 • edited

Codecov Report

NicolasGensollen left a comment

Choose a reason for hiding this comment

ymzayek commented Apr 26, 2023

mcsitter commented Apr 27, 2023

ymzayek commented Apr 28, 2023

mcsitter commented Apr 28, 2023

bthirion left a comment

Choose a reason for hiding this comment

ymzayek commented Jun 12, 2023 • edited

ymzayek commented Aug 2, 2023

mcsitter commented Aug 9, 2023

Remi-Gau commented Aug 9, 2023

ymzayek left a comment

Choose a reason for hiding this comment

Remi-Gau left a comment

Choose a reason for hiding this comment

ymzayek commented Aug 10, 2023

Remi-Gau Aug 10, 2023

Choose a reason for hiding this comment

ymzayek Aug 10, 2023

Choose a reason for hiding this comment

ymzayek left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 25, 2023 •

edited by Remi-Gau

codecov bot commented Apr 26, 2023 •

edited

ymzayek commented Jun 12, 2023 •

edited