[MAINT] remove leading underscore from non private functions #4086

Remi-Gau · 2023-10-27T12:59:45Z

Relates to Check for inconsistent naming of functions according to PEP8 convention #3319

Changes proposed in this pull request:

remove leading underscore from functions used outside of their own module in:
- nilearn/*.py
- nilearn/decoding
- nilearn/datasets
- nilearn/mass_univariate
rename module nilearn/decoding.objective_functions.py to nilearn/decoding._objective_functions.py
keep non user facing functions from nilearn/datasets/utils.py into a new file nilearn/datasets/_utils.py

github-actions · 2023-10-27T12:59:59Z

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

codecov · 2023-10-27T13:15:23Z

Codecov Report

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

Comparison is base (3ac5ebc) 91.81% compared to head (daf36d5) 91.68%.

Files	Patch %	Lines
nilearn/datasets/_utils.py	75.50%	69 Missing and 29 partials ⚠️
nilearn/datasets/func.py	84.61%	10 Missing ⚠️
nilearn/decoding/proximal_operators.py	84.61%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4086      +/-   ##
==========================================
- Coverage   91.81%   91.68%   -0.13%     
==========================================
  Files         144      145       +1     
  Lines       16241    16245       +4     
  Branches     3386     3386              
==========================================
- Hits        14911    14894      -17     
- Misses        789      807      +18     
- Partials      541      544       +3

Flag	Coverage Δ
macos-latest_3.10_test_plotting	`91.67% <82.94%> (+<0.01%)`	⬆️
macos-latest_3.11_test_plotting	`91.67% <82.94%> (+<0.01%)`	⬆️
macos-latest_3.12_test_plotting	`91.67% <82.94%> (+<0.01%)`	⬆️
macos-latest_3.8_test_plotting	`91.63% <82.58%> (+<0.01%)`	⬆️
macos-latest_3.9_test_plotting	`91.63% <82.58%> (+<0.01%)`	⬆️
ubuntu-latest_3.10_test_plotting	`91.67% <82.94%> (+<0.01%)`	⬆️
ubuntu-latest_3.11_test_plotting	`?`
ubuntu-latest_3.12_test_plotting	`?`
ubuntu-latest_3.12_test_pre	`?`
ubuntu-latest_3.8_test_plot_min	`?`
ubuntu-latest_3.8_test_plotting	`?`
ubuntu-latest_3.9_test_plotting	`?`
windows-latest_3.10_test_plotting	`?`
windows-latest_3.11_test_plotting	`?`
windows-latest_3.12_test_plotting	`?`
windows-latest_3.8_test_plotting	`?`
windows-latest_3.9_test_plotting	`?`

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

I generally see a bit of an issue with some files that I think should be named with a leading underscore (e.g. having functions in a file not named with a leading underscore that are used outside of that file but only within their respective parent module). So to me naming such functions with a leading underscore was done to signal that they are private to that module. So in short I think it might be worth discussing how to reconcile the way certain modules are structured with the guidelines we defined.

ymzayek · 2023-10-27T13:45:07Z

nilearn/datasets/utils.py

Maybe we should make this utils file private (rename to _utils.py). Only to make it clear that these functions are not part of the public API (exposed in docs) even though they are public in terms of the structure we defined.

On second thought that could break user code. The problem is that this file has 2 functions (get_data_dirs, load_sample_motor_activation_image) exposed in the docs which we cannot just change without deprecations but for the ones with names starting with underscores, I'm wondering how we signal that these can change at any moment without warning.

create a _utils.py module where we put the private stuff?

Sure if it's that simple I like it

I guess we would have to make it uber explicit in the doc string of the modules why there is a _utils and utils in the same folder...

nilearn/decoding/proximal_operators.py

Remi-Gau · 2023-10-27T14:22:45Z

So in short I think it might be worth discussing how to reconcile the way certain modules are structured with the guidelines we defined.

fine though I think the function level naming rule would still apply even if we renamed the module where it comes from

nilearn/decoding/objective_functions.py

Remi-Gau · 2023-10-27T15:11:59Z

will make the change we agreed on and will probably not do much more in this PR to keep things reviewable

Remi-Gau · 2023-10-27T15:29:28Z

nilearn/plotting/html_surface.py

@@ -150,7 +150,7 @@ def _full_brain_info(volume_img, mesh='fsaverage5', threshold=None,

    """
    info = {}
-    mesh = surface.surface._check_mesh(mesh)
+    mesh = surface.surface.check_mesh(mesh)


by the way I am assuming that usage like this mean that the function is used "outside" of its module because one could have done

from nilearn.surface.surface import _check_mesh

Remi-Gau · 2023-10-27T16:00:38Z

in a separate PR:

rename either nilearn.datasets.utils._fetch_file or nilearn.datasets.utils._fetch_files

but having both of them differing by a single letter is just not OK

Remi-Gau · 2023-10-27T16:21:14Z

nilearn/datasets/_utils.py

OK the number of line changed blew up because of splitting this file in 2

doc/changes/0.10.1.rst

Remi-Gau · 2023-10-28T09:27:24Z

nilearn/decoding/_objective_functions.py

@@ -225,7 +225,7 @@ def _sigmoid(t, copy=True):
    return t


-def _logistic(X, y, w):
+def logistic_loss(X, y, w):


note the function name change

Remi-Gau · 2023-10-28T09:27:50Z

nilearn/decoding/_objective_functions.py



-def _div(v):
+def divergence(v):


note the function name change

Remi-Gau · 2023-10-28T09:30:56Z

nilearn/decoding/_objective_functions.py

@@ -122,7 +122,7 @@ def _tv_l1_from_gradient(spatial_grad):
    return l1_term + tv_term


-def _div_id(grad, l1_ratio=0.5):
+def divergence_id(grad, l1_ratio=0.5):


note the function name change

Remi-Gau · 2023-10-28T10:54:38Z

Eventhough the functions changed here are private and therefore should not be used by users, I think that those changes should be mentioned in the changelog because I would be surprised if not a single users used some of those utility functions directly in their code.

any thoughts?

ymzayek

@Remi-Gau the PR LGTM!

One thing I'm a bit unsure about is that now in some cases as in the change of _check_mesh to check_mesh, how do we signal to the user and future developers that this function should be used internally (which also implies they can change without deprecation)? You can argue that it is not exposed in the API reference docs but some people don't look at that. One way could be to not include such functions in __all__ and explicitly use the module.file name when calling the function as is now done with check_mesh. But then we'd need to make this consistent across modules (with the exceptions being _utils) making the content of __all__ consistent with the API references in the docs. I'm not sure if this is the way to go so I'm happy to hear other thoughts

Remi-Gau · 2023-11-08T11:17:22Z

For context worth checking what pep8 says:
https://peps.python.org/pep-0008/#public-and-internal-interfaces

how do we signal to the user and future developers that this function should be used internally (which also implies they can change without deprecation)?

I agree that, unless I misunderstand something about PEP8, it feels that users would always need to go and look into the "all" of the init of a module to figure out if a given function is public or private: not convenient.

You can argue that it is not exposed in the API reference docs but some people don't look at that.

Well then in that case, it's not on us. We cannot control what people do or do not do. We can control how clear our doc is.

One way could be to not include such functions in __all__

yes

and explicitly use the module.file name when calling the function as is now done with check_mesh.

So do not do

from surface import check_mesh

check_mesh(foo)

but do

import surface

surface.check_mesh(foo)

If so that still does not signal that check_mesh is private.

But then we'd need to make this consistent across modules (with the exceptions being _utils) making the content of __all__ consistent with the API references in the docs

In any case I would be in favor of this.

Remi-Gau · 2023-11-09T12:40:48Z

FYI: we should expect a few more PRs like this one as this only touches a few modules

* update DOI * update which DOI is used and add RRID

Co-authored-by: Remi-Gau <Remi-Gau@users.noreply.github.com>

…le (nilearn#4049) * Correct linking * Fix note

* expose CubicSpline extrapolate parameter (nilearn#4028) * [FIX] discard non-interpolated volumes and reset the sample_mask indexes * [REF] discard non-interpolated volumes and reset sample_mask in '_handle_scrubbed_volumes' * [FMT] black formatting * improve deprecation warning message * check that extrapolation warning is raised * add changelog and contribution entries * check consistency of modified sample_mask * assert modified sample_mask can be applied to signals and confounds * [REF] split signal extrapolation testing in 2 tests

Co-authored-by: ymzayek <ymzayek@users.noreply.github.com>

* Add listing utilities * Add possibility to filter by modules * Add some tests * Fix PEP8 * Clarify ignored modules by default * Add section on Nilearn API in the user guide * Export at the end * [circle full] Add whats new * revert * more revert * lint * fix tests * lint * adapt tests to check number of public functions and classes * update doc strings * fix --------- Co-authored-by: Remi Gau <remi_gau@hotmail.com>

Remi-Gau · 2023-11-09T15:32:03Z

Ok the listing utilities test is doing its job as "number of public function change detector" is working as intended as shown by the latest failures.

Remi-Gau · 2023-12-07T15:24:15Z

Unless anyone has second thoughts about this one, I will merge it tomorrow and then open separate issues + PR for the things mentioned in here.

remove leading underscore from non private functions

a6c0f6f

Remi-Gau added 2 commits October 27, 2023 15:15

rm underscores in datasets

f8585ee

rm underscores in decoding

8d189ae

ymzayek reviewed Oct 27, 2023

View reviewed changes

nilearn/decoding/objective_functions.py Outdated Show resolved Hide resolved

Remi-Gau added 2 commits October 27, 2023 17:18

update script

9120add

rename module

520cfeb

Remi-Gau commented Oct 27, 2023

View reviewed changes

Remi-Gau added 4 commits October 27, 2023 18:05

rm more leading underscore from decoding mass_univariate datasets

08bbf9b

rename module

75eeac9

move functions

f352bde

add doc string

92ab7f0

Remi-Gau commented Oct 27, 2023

View reviewed changes

nilearn/datasets/_utils.py Outdated

Copy link

Collaborator Author

Remi-Gau Oct 27, 2023

Choose a reason for hiding this comment

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

OK the number of line changed blew up because of splitting this file in 2

rename functions

96afcaa

Remi-Gau commented Oct 28, 2023

View reviewed changes

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

Remi-Gau commented Oct 28, 2023

View reviewed changes

nilearn/decoding/_objective_functions.py

def _div(v):

def divergence(v):

Copy link

Collaborator Author

Remi-Gau Oct 28, 2023

Choose a reason for hiding this comment

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

note the function name change

Remi-Gau added 2 commits October 28, 2023 11:29

rename function

a981080

Update doc/changes/0.10.1.rst

0571abf

Remi-Gau commented Oct 28, 2023

View reviewed changes

flake8

a5d456a

fix

3f52a51

Remi-Gau marked this pull request as ready for review October 28, 2023 11:09

Remi-Gau requested a review from ymzayek October 28, 2023 11:09

ymzayek approved these changes Nov 8, 2023

View reviewed changes

Remi-Gau and others added 12 commits November 9, 2023 13:44

typo (nilearn#4091)

80f8058

[MAINT] update DOI and add RRID (nilearn#4032)

40716ec

* update DOI * update which DOI is used and add RRID

[DOC] mention checking badges during release (nilearn#4085)

96fde51

[MAINT] Fix Zenodo DOI badge (nilearn#4093)

874ce8b

pre-commit hooks auto-update (nilearn#4097)

f4f3606

Co-authored-by: Remi-Gau <Remi-Gau@users.noreply.github.com>

Remove extraneous hash symbols (nilearn#4096)

7cc1d7d

[DOC] Improve docstring rendering for typed experimental surface modu…

27efe94

…le (nilearn#4049) * Correct linking * Fix note

Add tagging (nilearn#4100)

f9f4336

[BOT] update AUTHORS.rst and doc/changes/names.rst (nilearn#4102)

259b005

Co-authored-by: ymzayek <ymzayek@users.noreply.github.com>

Merge remote-tracking branch 'upstream/main' into check_private

32c500a

Remi-Gau added 4 commits November 9, 2023 16:34

fix number of functions

e9b82bb

Merge remote-tracking branch 'upstream/main' into check_private

70783c7

adapt number of functions

1b30a5b

update changelog

daf36d5

Remi-Gau merged commit 439e436 into nilearn:main Dec 9, 2023
31 of 32 checks passed

Remi-Gau deleted the check_private branch December 9, 2023 08:23

liuzhenqi77 mentioned this pull request Feb 1, 2024

[FIX] Fix interface changes from nilearn netneurolab/neuromaps#146

Merged

Remi-Gau mentioned this pull request Feb 2, 2024

nilearn 0.10.3 breaks nimare neurostuff/NiMARE#872

Closed

ghisvail mentioned this pull request Feb 2, 2024

Not compatible with nilearn version 0.10.1 onwards netneurolab/netneurotools#132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINT] remove leading underscore from non private functions #4086

[MAINT] remove leading underscore from non private functions #4086

Remi-Gau commented Oct 27, 2023 •

edited

github-actions bot commented Oct 27, 2023

codecov bot commented Oct 27, 2023 •

edited

ymzayek left a comment

ymzayek Oct 27, 2023

ymzayek Oct 27, 2023

Remi-Gau Oct 27, 2023

ymzayek Oct 27, 2023

Remi-Gau Oct 27, 2023

Remi-Gau commented Oct 27, 2023

Remi-Gau commented Oct 27, 2023

Remi-Gau Oct 27, 2023

Remi-Gau commented Oct 27, 2023

Remi-Gau Oct 27, 2023

Remi-Gau Oct 28, 2023

Remi-Gau Oct 28, 2023

Remi-Gau Oct 28, 2023

Remi-Gau commented Oct 28, 2023

ymzayek left a comment

Remi-Gau commented Nov 8, 2023

Remi-Gau commented Nov 9, 2023

Remi-Gau commented Nov 9, 2023

Remi-Gau commented Dec 7, 2023

[MAINT] remove leading underscore from non private functions #4086

[MAINT] remove leading underscore from non private functions #4086

Conversation

Remi-Gau commented Oct 27, 2023 • edited

github-actions bot commented Oct 27, 2023

codecov bot commented Oct 27, 2023 • edited

Codecov Report

ymzayek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Oct 27, 2023

Remi-Gau commented Oct 27, 2023

Choose a reason for hiding this comment

Remi-Gau commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Oct 28, 2023

ymzayek left a comment

Choose a reason for hiding this comment

Remi-Gau commented Nov 8, 2023

Remi-Gau commented Nov 9, 2023

Remi-Gau commented Nov 9, 2023

Remi-Gau commented Dec 7, 2023

Remi-Gau commented Oct 27, 2023 •

edited

codecov bot commented Oct 27, 2023 •

edited