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

Remove auto argument from pm.Deterministic docstring #6592

Merged
merged 4 commits into from
Mar 14, 2023

Conversation

shreyas3156
Copy link
Contributor

What is this PR about?
Closes #6588
Removed the auto argument from the docstring of pm.Deterministic as it was removed in the cleanup in #6309

Checklist

@michaelosthege
Copy link
Member

michaelosthege commented Mar 12, 2023

Can you add the missing ones and format it according to the numpydoc style guide?

@shreyas3156
Copy link
Contributor Author

Can you add the missing ones and format it according to the [numpydoc style guide] (https://numpydoc.readthedocs.io/en/latest/format.html#parameters)?

Sure! I'll add the params model and dims. I also noticed that Potential does not have dims in the docstring either. Shall I add it there too?

@michaelosthege
Copy link
Member

sure

@larryshamalama
Copy link
Member

larryshamalama commented Mar 12, 2023

Thanks @shreyas3156 for this PR!

Can you add the missing ones and format it according to the numpydoc style guide?

Just for some additional pointers, you can take inspiration from some of the docstrings that are available in the same file:

pymc/pymc/model.py

Lines 1921 to 1922 in b5fa488

model : Model, optional
Current model on stack.

pymc/pymc/model.py

Lines 1300 to 1301 in b5fa488

dims: tuple
Dimension names for the variable.

(A note aside is that I'm noticing that many of other docstrings could be updated... that'll be for another issue)

Edit: Didn't see the fast responses above when typing this...

@codecov
Copy link

codecov bot commented Mar 12, 2023

Codecov Report

Merging #6592 (3298083) into main (cf6d4ce) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6592   +/-   ##
=======================================
  Coverage   92.03%   92.03%           
=======================================
  Files          92       92           
  Lines       15535    15539    +4     
=======================================
+ Hits        14297    14302    +5     
+ Misses       1238     1237    -1     
Impacted Files Coverage Δ
pymc/distributions/discrete.py 99.22% <ø> (ø)
pymc/model.py 89.52% <ø> (ø)

... and 4 files with indirect coverage changes

@shreyas3156
Copy link
Contributor Author

Just for some additional pointers, you can take inspiration from some of the docstrings that are available in the same file:

Thank you for the reference!

@michaelosthege @larryshamalama I've added the args and made a few changes as per the style guide. Please review and let me know your feedback.

pymc/model.py Outdated Show resolved Hide resolved
Copy link
Member

@larryshamalama larryshamalama left a comment

Choose a reason for hiding this comment

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

Thanks you for this PR @shreyas3156! You're on a roll with these PRs!

There's a slight pre-commit error, but I believe that it was recently introduced and fixed. If you're not familiar with pre-commit, @ricardoV94 gave me some nice pointers 2 years ago that helped a lot. I'm just mentioning this in case this could also help you later down the line :)

@shreyas3156
Copy link
Contributor Author

There's a slight pre-commit error, but I believe that it was recently introduced and fixed. If you're not familiar with pre-commit, @ricardoV94 gave me some nice pointers 2 years ago that helped a lot. I'm just mentioning this in case this could also help you later down the line :)

Thank you for this suggestion! I could get a better context of pre-commit after going through it.

Thanks you for this PR @shreyas3156! You're on a roll with these PRs!

Glad I could contribute :)

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

Yes, that whitespace change was fixed on another PR and we can override here.

Thanks @shreyas3156 !

@michaelosthege michaelosthege merged commit c709abf into pymc-devs:main Mar 14, 2023
@shreyas3156 shreyas3156 deleted the docs-deterministic-6588 branch April 23, 2023 18:34
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.

Remove auto argument from pm.Deterministic docstring
3 participants