[DOC] Improve docstring rendering for typed experimental surface module #4049

ymzayek · 2023-10-12T09:42:42Z

Closes [DOC] Fix Sphinx issues related to adding new experimental module and typing #4018

~~Changes proposed in this pull request:~~ EDIT see #4049 (comment) below for what is covered in this PR

Add some configs to doc/conf.py for typehints
I'm using nilearn/experimental/surface/_maskers.py to test some options and after some decisions are made changes will be applied to the rest of this module. I'll add some comments and images to show differences of rendering.

There are two main things to pay attention to. I tried just removing the type from the docstring and let it render in the signature so as to not repeat information. This leaves the parameter names and the description. The other way would be to change to way we write docstrings but it renders better. See docstring of SurfaceLabelsMasker.fit_transform().

ymzayek · 2023-10-12T09:45:46Z

nilearn/experimental/surface/_maskers.py

+    #: Labels array concatenated across both hemispheres
+    labels_data_: np.ndarray
+    #: Unique label values
+    labels_: np.ndarray
+    #: Label names if any, otherwise unique label values
+    label_names_: np.ndarray


attributes can just have this #: syntax with a description above each (which will render under) and no need for an attributes section in the docstring. See e.g.

The pro is that the documentation is closer to where the attributes are declared in the code but this would mean rewriting a lot of doc, no?

ymzayek · 2023-10-12T09:49:16Z

nilearn/experimental/surface/_maskers.py

            Mesh and data for both hemispheres.

-        y : None
+        y
            This parameter is unused. It is solely included for scikit-learn
            compatibility.



rendering of SurfaceLabelsMasker.fit ( we would render the input parameter types in the signature for this option but I somehow messed up the configs here so use your imagination :) )

ymzayek · 2023-10-12T09:52:22Z

nilearn/experimental/surface/_maskers.py

-            Signal for each element.
-            shape: (img data shape, total number of vertices)
+        :return: Signal for each element with shape
+                 (img data shape, total number of vertices).
        """


SurfaceLabelsMasker.fit_transform with docstring compatible with sphinx autdoc typehints to get the types and defaults automatically added in the docstring instead of the signature (I think can also be done for the return if we want to keep it out of the signature):

ymzayek · 2023-10-12T09:56:51Z

doc/modules/experimental.rst

+.. automodule:: nilearn.experimental.surface
+   :no-members:
+   :no-inherited-members:


this is actually needed to correctly reference the module and wasn't causing the :noindex: error in #4017

github-actions · 2023-10-12T10:23:41Z

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

codecov · 2023-10-12T10:39:12Z

Codecov Report

Merging #4049 (c821277) into main (d8fc894) will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4049      +/-   ##
==========================================
- Coverage   91.59%   91.51%   -0.09%     
==========================================
  Files         143      143              
  Lines       16079    16079              
  Branches     3340     3340              
==========================================
- Hits        14728    14715      -13     
- Misses        804      819      +15     
+ Partials      547      545       -2

Flag	Coverage Δ
macos-latest_3.10	`91.51% <ø> (?)`
macos-latest_3.11	`91.51% <ø> (ø)`
macos-latest_3.12	`91.51% <ø> (ø)`
macos-latest_3.9	`91.47% <ø> (ø)`
ubuntu-latest_3.10	`91.51% <ø> (ø)`
ubuntu-latest_3.11	`91.51% <ø> (ø)`
ubuntu-latest_3.12	`91.51% <ø> (?)`
ubuntu-latest_3.9	`91.47% <ø> (ø)`
windows-latest_3.10	`91.46% <ø> (ø)`
windows-latest_3.11	`91.46% <ø> (?)`
windows-latest_3.12	`91.46% <ø> (?)`
windows-latest_3.9	`91.43% <ø> (ø)`

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

see 5 files with indirect coverage changes

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

ymzayek · 2023-10-12T10:42:27Z

FWIW I did try Napoleon with the napoleon typehints extension but ran into a bunch of issues. While it has some nice features and can better standardize our docstrings, it seem to be incompatible with our current docstrings as they are so will either require some work to update those in the whole codebase or some more intense configs and workarounds. In contrast, the configs in this PR do not affect the docstrings of untyped code. And at least the decisions/issues resolved in this PR also seem to apply to and be compatible with Napoleon so someone can still switch to it later if desired

Remi-Gau · 2023-10-13T06:37:19Z

Thanks for looking into this @ymzayek !!

Some options would make the docstring writing much easier.

Given this is related to type hints, should we maybe add this to the agenda for the next dev meeting?

This may also help solve how to handle the PR and issues related to where to put "defaults" in the doc string.

ymzayek · 2023-10-13T07:57:00Z

Given this is related to type hints, should we maybe add this to the agenda for the next dev meeting?

@Remi-Gau yes definitely, I've put it on the agenda under issues/PRs

What I'm struggling with is trying to merge both approaches, i.e. having numpydoc style docstring in the codebase but substituting types (and defaults) automatically without doubling the parameters. The problem with the use of #: and :param ... syntax is that it is not as readable in the code. I'm not quite sure yet if the support for typing is lacking or that I haven't figured out the right mix of configs and changes to the docstrings.

ymzayek · 2023-10-30T19:15:59Z

Update: Now this PR deals with only points 1 and 2 of #4018.
Since we decided not to support typing at the moment, we can deal with its rendering at a later point since how we implement the typing will influence the configuration.

bthirion

LGTM, thx.

Remi-Gau

Thanks for looking into all this @ymzayek

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

* remove leading underscore from non private functions * rm underscores in datasets * rm underscores in decoding * update script * rename module * rm more leading underscore from decoding mass_univariate datasets * rename module * move functions * add doc string * rename functions * rename function * Update doc/changes/0.10.1.rst * flake8 * fix * Update nilearn/signal.py Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com> * typo (#4091) * [MAINT] update DOI and add RRID (#4032) * update DOI * update which DOI is used and add RRID * [DOC] mention checking badges during release (#4085) * [MAINT] Fix Zenodo DOI badge (#4093) * pre-commit hooks auto-update (#4097) Co-authored-by: Remi-Gau <Remi-Gau@users.noreply.github.com> * Remove extraneous hash symbols (#4096) * [DOC] Improve docstring rendering for typed experimental surface module (#4049) * Correct linking * Fix note * Add tagging (#4100) * [FIX] Disable extrapolation of out-of-bounds volumes (#4028) * expose CubicSpline extrapolate parameter (#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 * [BOT] update AUTHORS.rst and doc/changes/names.rst (#4102) Co-authored-by: ymzayek <ymzayek@users.noreply.github.com> * [MAINT] Add listing utilities (#2991) * 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> * fix number of functions * adapt number of functions * update changelog --------- Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Remi-Gau <Remi-Gau@users.noreply.github.com> Co-authored-by: Jordi Huguet <jhuguetn@gmail.com> Co-authored-by: ymzayek <ymzayek@users.noreply.github.com> Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

ymzayek commented Oct 12, 2023

View reviewed changes

Remi-Gau mentioned this pull request Oct 15, 2023

[DOC] move default from end of argument description to type name #4040

Merged

Correct linking

de6d168

ymzayek force-pushed the improve-surface-docstrings branch from 1a1a607 to de6d168 Compare October 30, 2023 19:09

Fix note

c821277

ymzayek marked this pull request as ready for review October 30, 2023 19:16

ymzayek requested review from Remi-Gau and bthirion November 6, 2023 08:03

bthirion approved these changes Nov 6, 2023

View reviewed changes

Remi-Gau approved these changes Nov 7, 2023

View reviewed changes

ymzayek merged commit aca8646 into nilearn:main Nov 7, 2023
29 checks passed

ymzayek deleted the improve-surface-docstrings branch November 7, 2023 12:07

Remi-Gau pushed a commit to Remi-Gau/nilearn that referenced this pull request Nov 9, 2023

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

27efe94

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DOC] Improve docstring rendering for typed experimental surface module #4049

[DOC] Improve docstring rendering for typed experimental surface module #4049

ymzayek commented Oct 12, 2023 •

edited

ymzayek Oct 12, 2023

Remi-Gau Oct 13, 2023

ymzayek Oct 12, 2023

ymzayek Oct 12, 2023

ymzayek Oct 12, 2023

github-actions bot commented Oct 12, 2023

codecov bot commented Oct 12, 2023 •

edited

ymzayek commented Oct 12, 2023

Remi-Gau commented Oct 13, 2023

ymzayek commented Oct 13, 2023

ymzayek commented Oct 30, 2023

bthirion left a comment

Remi-Gau left a comment

[DOC] Improve docstring rendering for typed experimental surface module #4049

[DOC] Improve docstring rendering for typed experimental surface module #4049

Conversation

ymzayek commented Oct 12, 2023 • edited

ymzayek Oct 12, 2023

Choose a reason for hiding this comment

Remi-Gau Oct 13, 2023

Choose a reason for hiding this comment

ymzayek Oct 12, 2023

Choose a reason for hiding this comment

ymzayek Oct 12, 2023

Choose a reason for hiding this comment

ymzayek Oct 12, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 12, 2023

codecov bot commented Oct 12, 2023 • edited

Codecov Report

ymzayek commented Oct 12, 2023

Remi-Gau commented Oct 13, 2023

ymzayek commented Oct 13, 2023

ymzayek commented Oct 30, 2023

bthirion left a comment

Choose a reason for hiding this comment

Remi-Gau left a comment

Choose a reason for hiding this comment

ymzayek commented Oct 12, 2023 •

edited

codecov bot commented Oct 12, 2023 •

edited