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

DOC: Revision of docstrings in the iterators' module #29

Merged
merged 5 commits into from
Jan 23, 2025

Conversation

jhlegarreta
Copy link
Contributor

@jhlegarreta jhlegarreta commented Dec 22, 2024

  • ENH: Add None to b-vals iterators size parameter annotations
  • ENH: Make linear_iterator return an iterator
  • ENH: Remove unused parameter form bvalue_iterator prototype

@jhlegarreta jhlegarreta force-pushed the FixbvalIteratorsTypeAnnotations branch from f29739a to 539250c Compare December 22, 2024 01:07
Copy link

codecov bot commented Dec 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.24%. Comparing base (d9b390b) to head (8ceaf57).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
+ Coverage   68.10%   68.24%   +0.13%     
==========================================
  Files          20       20              
  Lines         994      995       +1     
  Branches      130      130              
==========================================
+ Hits          677      679       +2     
  Misses        263      263              
+ Partials       54       53       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jhlegarreta jhlegarreta marked this pull request as ready for review December 22, 2024 01:20
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Dec 22, 2024

@oesteban
Copy link
Member

There seems to be some conflicts after the style update I made.

Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

I don't see why the iter() decoration to range, other than that looks good!

@jhlegarreta jhlegarreta force-pushed the FixbvalIteratorsTypeAnnotations branch from 87fed5d to 3ebda3a Compare January 14, 2025 13:57
@jhlegarreta
Copy link
Contributor Author

There seems to be some conflicts after the style update I made.

Force pushed; conflicts gone.

Add `None` to b-vals iterators `size` parameter annotations.

Edit the parameter types accordingly in the docstrings and describe what
happens when `None` is provided.

Fixes:
```
src/nifreeze/utils.py:30: error:
 Incompatible default for argument "size" (default has type "None", argument has type "int")  [assignment]
src/nifreeze/utils.py:30: note:
 PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
src/nifreeze/utils.py:30: note:
 Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
src/nifreeze/utils.py:59: error:
 Incompatible default for argument "size" (default has type "None", argument has type "int")  [assignment]
src/nifreeze/utils.py:59:
 note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
src/nifreeze/utils.py:59:
 note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
src/nifreeze/utils.py:108:
 error: Incompatible default for argument "size" (default has type "None", argument has type "int")  [assignment]
src/nifreeze/utils.py:108:
 note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
src/nifreeze/utils.py:108:
 note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
src/nifreeze/utils.py:135:
 error: Incompatible default for argument "size" (default has type "None", argument has type "int")  [assignment]
src/nifreeze/utils.py:135:
 note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
src/nifreeze/utils.py:135:
 note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
```

raised for example in:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:32
Remove unused parameter from method `bvalue_iterator` prototype and
docstring. Use the `*_` to denote unused arguments.
@oesteban oesteban force-pushed the FixbvalIteratorsTypeAnnotations branch from 3ebda3a to 8e7aba9 Compare January 23, 2025 08:10
@oesteban oesteban changed the title ENH: Fix type annotations in b-vals iterators DOC: Revision of docstrings in the iterators' module Jan 23, 2025
@oesteban
Copy link
Member

In 6398249, I have added the following changes:

  • Revert the removal of optional keyword arguments from docstrings. The rationale is that, if they are not documented there, the type hints in the signature are insufficient to introspect what keyword arguments may be provided and their behavior. As discussed above, you can use Other parameters for this as per numpydoc. However, in this particular case I find it overly verbose. Instead, I propose we explicitly label these arguments as optional, keyword argument.
  • Rewrite the Returns section with Yields, following numpydocs' documentation, as all these are meant to yield values rather than return values.

In addition to those changes, I also rolled back the iter() decoration of the linear iterator, and replace it with an actual generator, for consistency with the other iterators.

You can always convert these generators to iterators or lists at any moment.

@jhlegarreta @effigies let me know if the above sounds reasonable.

Copy link
Contributor Author

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

In 6398249, I have added the following changes:

  • Revert the removal of optional keyword arguments from docstrings. The rationale is that, if they are not documented there, the type hints in the signature are insufficient to introspect what keyword arguments may be provided and their behavior. As discussed above, you can use Other parameters for this as per numpydoc. However, in this particular case I find it overly verbose. Instead, I propose we explicitly label these arguments as optional, keyword argument.

Agreed that documenting these is useful in this case. Left a few style comment to make documenting these nicer/more Pythonin/Sphinxier if there is a way.

  • Rewrite the Returns section with Yields, following numpydocs' documentation, as all these are meant to yield values rather than return values.

👍 Yield looks the right thing.

In addition to those changes, I also rolled back the iter() decoration of the linear iterator, and replace it with an actual generator, for consistency with the other > iterators.

OK.

@jhlegarreta @effigies let me know if the above sounds reasonable.

Changes look OK. The only inconsistency is the _iterator naming vs. them being generators.

Co-authored-by: Jon Haitz Legarreta Gorroño <jon.haitz.legarreta@gmail.com>
@oesteban
Copy link
Member

The only inconsistency is the _iterator naming vs. them being generators.

Let's then punt this inconsistency for the future, as the module is also called "iterators". They are indeed iterators (that's why the recommendation for the type hint of the return is Iterator), however they are not implemented exactly like proper Python iterators.

If the naming is too irritating for you or anyone, happy to see a future PR addressing this.

@oesteban oesteban merged commit e44074f into nipreps:main Jan 23, 2025
8 of 9 checks passed
oesteban added a commit that referenced this pull request Jan 23, 2025
oesteban added a commit that referenced this pull request Jan 23, 2025
FIX: Test with a warning unhandled by #29
@jhlegarreta jhlegarreta deleted the FixbvalIteratorsTypeAnnotations branch January 23, 2025 23:37
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.

2 participants