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: Fix docstrings in gradient.py #415

Merged
merged 2 commits into from
Sep 5, 2023
Merged

DOC: Fix docstrings in gradient.py #415

merged 2 commits into from
Sep 5, 2023

Conversation

hsinfan1996
Copy link
Contributor

@hsinfan1996 hsinfan1996 commented Aug 16, 2023

Motivation for these changes

Fix docstrings that are not formatted correctly.
Closes #414

Implementation details

According to https://numpydoc.readthedocs.io/en/latest/format.html#returns, return type is always required. Missing return types are added to gradient.py grad Lop and Rop.

Checklist

Major / Breaking Changes

  • No

New features

  • No

Bugfixes

  • No

Documentation

  • ...

Maintenance

  • ...

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

This is an improvement, but the docstring might need a more thorough review. In the description (line 205) it says:

"If wrt is a list/tuple, then return a list/tuple with the results."

But in lines 355-357 it's f what is checked to match its output type. I also don't know how common Variable is, it might be worth to use https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html#confval-napoleon_type_aliases for Variable so it is easier to write but still keep the hyperlink.

@hsinfan1996
Copy link
Contributor Author

Actually I am not too familiar with the code itself.

@ricardoV94
Copy link
Member

"If wrt is a list/tuple, then return a list/tuple with the results."

You're right. This is correct for the grad but not L_op and R_op, so we should fix the docstrings of those.

I also don't know how common Variable is,

Quite common, after TensorVariable. @hsinfan1996 is this something you want to take a stab at? No problem if not, we can open a separate issue for it.

@hsinfan1996
Copy link
Contributor Author

hsinfan1996 commented Aug 17, 2023

For L_op and R_op, if it is as simple as removing line 205 and 394, then I can do it. If not, I am afraid I will not be able to take care of the rest.

@ricardoV94
Copy link
Member

ricardoV94 commented Aug 17, 2023

For L_op and R_op, if it is as simple as removing line 205 and 394, then I can do it. If not, I am afraid I will not be able to take care of the rest.

I think the correct thing would be: "If f is a list/tuple, then return a list/tuple with the results."

@hsinfan1996
Copy link
Contributor Author

For L_op and R_op, if it is as simple as removing line 205 and 394, then I can do it. If not, I am afraid I will not be able to take care of the rest.

I think the correct thing would be: "If f is a list/tuple, then return a list/tuple with the results."

Got it.

@hsinfan1996
Copy link
Contributor Author

Is there something preventing us from proceeding with this PR?

@ricardoV94
Copy link
Member

Is there something preventing us from proceeding with this PR?

I didn't realize you had fixed the Rop already. Should be good to go

@ricardoV94 ricardoV94 merged commit 243836e into pymc-devs:main Sep 5, 2023
51 of 52 checks passed
@ricardoV94
Copy link
Member

Thanks @hsinfan1996

@ricardoV94 ricardoV94 changed the title DOC: Fix docstrings in gradient.py DOC: Fix docstrings in gradient.py Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: wrong indentation level in gradient.py grad Lop and Rop
3 participants