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

[MAINT] Rewrite write_tmp_imgs function and use it with pytest's tmp_path #4094

Merged
merged 7 commits into from Nov 13, 2023

Conversation

ymzayek
Copy link
Member

@ymzayek ymzayek commented Oct 30, 2023

Relates to #3935 (comment)

Changes proposed in this pull request:

  • The function now looks like other testing helper functions that write data to disk by accepting a file_path argument. Now unit tests can pass it the pytest tmp_path fixture letting pytest take care of setting up a tmp directory and cleaning up the files.

@github-actions
Copy link
Contributor

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

nilearn/_utils/testing.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #4094 (2acb067) into main (9ed165b) will increase coverage by 0.03%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4094      +/-   ##
==========================================
+ Coverage   91.59%   91.63%   +0.03%     
==========================================
  Files         143      143              
  Lines       16128    16115      -13     
  Branches     3357     3353       -4     
==========================================
- Hits        14773    14767       -6     
+ Misses        806      800       -6     
+ Partials      549      548       -1     
Flag Coverage Δ
macos-latest_3.10 ?
macos-latest_3.11 91.54% <100.00%> (+0.03%) ⬆️
macos-latest_3.12 ?
macos-latest_3.9 ?
ubuntu-latest_3.10 91.54% <100.00%> (+0.03%) ⬆️
ubuntu-latest_3.11 91.54% <100.00%> (+0.03%) ⬆️
ubuntu-latest_3.12 ?
ubuntu-latest_3.9 91.51% <100.00%> (+0.03%) ⬆️
windows-latest_3.10 ?
windows-latest_3.11 91.50% <100.00%> (+0.03%) ⬆️
windows-latest_3.12 ?
windows-latest_3.9 91.47% <100.00%> (+0.03%) ⬆️

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

Files Coverage Δ
nilearn/_utils/testing.py 79.01% <100.00%> (+4.54%) ⬆️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

Except for the name change it looks good to me.

Thanks a bunch for taking care of this one.

Realizing that refactoring this context manager had been on my mind for a while, so defo happy to see it go.

@ymzayek
Copy link
Member Author

ymzayek commented Nov 10, 2023

I would like to rebase #3935 on this so hoping to merge soon. Another review would be great!

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.

I'm mostly happy with the PR, but ---again--- with the naming change from write_tmp_imgs to ẁrite_fake_imgs, as there is nothing in this function that implies that images are fake.

nilearn/_utils/testing.py Outdated Show resolved Hide resolved
nilearn/_utils/testing.py Outdated Show resolved Hide resolved
@Remi-Gau
Copy link
Collaborator

I guess that we could just call it write_imgs as they are not "fake" but they are not temporary unless when written in a tmp_dir

@ymzayek
Copy link
Member Author

ymzayek commented Nov 13, 2023

Yes makes sense. What about write_imgs_to_path?

@ymzayek
Copy link
Member Author

ymzayek commented Nov 13, 2023

I guess that we could just call it write_imgs as they are not "fake" but they are not temporary unless when written in a tmp_dir

Or yea just this one

@Remi-Gau
Copy link
Collaborator

either are ok with me

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.

LGTM, thx !

@ymzayek ymzayek merged commit aa9c914 into nilearn:main Nov 13, 2023
29 checks passed
@ymzayek ymzayek deleted the rework_write_tmp_imgs branch November 13, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants