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

[FIX] rename contrast_type to stat_type #4191

Merged
merged 10 commits into from Jan 9, 2024

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Jan 5, 2024

Changes proposed in this pull request:

Copy link
Contributor

github-actions bot commented Jan 5, 2024

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

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

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

Comparison is base (91e59cc) 92.10% compared to head (137fb35) 92.11%.

Files Patch % Lines
nilearn/glm/contrasts.py 87.09% 0 Missing and 4 partials ⚠️
nilearn/glm/_utils.py 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4191   +/-   ##
=======================================
  Coverage   92.10%   92.11%           
=======================================
  Files         144      144           
  Lines       16358    16366    +8     
  Branches     3423     3426    +3     
=======================================
+ Hits        15067    15075    +8     
  Misses        752      752           
  Partials      539      539           
Flag Coverage Δ
macos-latest_3.10_test_plotting ?
macos-latest_3.11_test_plotting ?
macos-latest_3.12_test_plotting 91.89% <86.84%> (+<0.01%) ⬆️
macos-latest_3.9_test_plotting 91.85% <86.84%> (+<0.01%) ⬆️
ubuntu-latest_3.10_test_plotting 91.89% <86.84%> (+<0.01%) ⬆️
ubuntu-latest_3.11_test_plotting 91.89% <86.84%> (+<0.01%) ⬆️
ubuntu-latest_3.12_test_plotting 91.89% <86.84%> (+<0.01%) ⬆️
ubuntu-latest_3.12_test_pre 91.89% <86.84%> (+<0.01%) ⬆️
ubuntu-latest_3.8_test_min 68.99% <86.84%> (+0.01%) ⬆️
ubuntu-latest_3.8_test_plot_min 91.61% <86.84%> (+<0.01%) ⬆️
ubuntu-latest_3.8_test_plotting 91.85% <86.84%> (+<0.01%) ⬆️
ubuntu-latest_3.9_test_plotting 91.85% <86.84%> (+<0.01%) ⬆️
windows-latest_3.10_test_plotting ?
windows-latest_3.11_test_plotting 91.86% <86.84%> (+<0.01%) ⬆️
windows-latest_3.12_test_plotting ?
windows-latest_3.8_test_plotting 91.82% <86.84%> (+<0.01%) ⬆️
windows-latest_3.9_test_plotting 91.83% <86.84%> (+<0.01%) ⬆️

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.

@bthirion
Copy link
Member

bthirion commented Jan 5, 2024

We'll have to go through deprecation cycles for that one...

Comment on lines +51 to +53
@rename_parameters(
replacement_params={"contrast_type": "stat_type"}, end_version="0.13.0"
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This decorator should take care of the deprecation cycle.

Will add a test to make sure of it.

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Jan 6, 2024

@bthirion
for consistency I would also change the attribute contrast_type in the Contrast class.

contrast_type="t",

What do you think?

@bthirion
Copy link
Member

bthirion commented Jan 6, 2024

Indeed, the idea is to have a uniform naming across classes and functions.

@Remi-Gau Remi-Gau changed the title [WIP][FIX] rename contrast_type to stat_type [FIX] rename contrast_type to stat_type Jan 6, 2024
@Remi-Gau Remi-Gau marked this pull request as ready for review January 6, 2024 09:11
@Remi-Gau Remi-Gau changed the title [FIX] rename contrast_type to stat_type [FIX] rename contrast_type to stat_type Jan 8, 2024
@@ -986,6 +986,7 @@ def first_level_from_bids(
sub_labels : :obj:`list` of :obj:`str`, optional
Specifies the subset of subject labels to model.
If 'None', will model all subjects in the dataset.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@ymzayek ymzayek left a comment

Choose a reason for hiding this comment

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

Thx @Remi-Gau ! The changes LGTM

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 !

@Remi-Gau Remi-Gau merged commit 44b2d6f into nilearn:main Jan 9, 2024
31 of 32 checks passed
@Remi-Gau Remi-Gau deleted the contrast_type_rename branch January 9, 2024 09:12
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.

[BUG] inconsistency in the naming
3 participants