-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Updated docstrings in pymc.step_methods.metropolis for BinaryGibbsMetropolis to follow numpydoc #7212
base: main
Are you sure you want to change the base?
Conversation
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see in the preview: https://pymcio--7212.org.readthedocs.build/projects/docs/en/7212/api/generated/classmethods/pymc.step_methods.Metropolis.__init__.html, this is already a great improvement. I have added some extra comments to try and get rid of the pink text in the type info and have everything as links.
The only general comment is to add the , optional
or default xyz
info too when relevant as covered in https://pymc-data-umbrella.xyz/en/latest/contributing/tutorials/docstring_tutorial.html#parameters
pymc/step_methods/metropolis.py
Outdated
List of value variables for sampler | ||
S: standard deviation or covariance matrix | ||
S : standard deviation or covariance matrix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
standard deviation or covariance matrix aren't types, this is mixing type with description. I think I have never used Metropolis, so my guess about behaviour here might be wrong but here is a proposal:
S : array_like with shape (N,) or (N, N), optional
Then in proposal_dist
description:
Function that returns zero-mean deviates when parameterized with
`S` (and n). If `S` is one dimensional, it defaults to a normal, with `S` as its standard deviation;
If `S` is two dimensional, it defaults to a multivariate normal with `S` as its covariance matrix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed this one based on your suggestions. I also changed S
description to match.
pymc/step_methods/metropolis.py
Outdated
Function that returns zero-mean deviates when parameterized with | ||
S (and n). Defaults to normal. | ||
scaling: scalar or array | ||
scaling : scalar or array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scaling : scalar or array | |
scaling : scalar or array_like, default 1 |
for inputs unless it is required they are already a numpy array it is better to use array_like instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed this one based on your suggestions.
pymc/step_methods/metropolis.py
Outdated
The frequency of tuning. Defaults to 100 iterations. | ||
model: PyMC Model | ||
model : PyMC Model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
model : PyMC Model | |
model : Model |
using only Model
should trigger the use of this alias: https://github.com/pymc-devs/pymc/blob/main/docs/source/conf.py#L73 and render that as a link to the pymc.Model api documentation.
note, it might not work right now because a while back some objects were undocumented. For example, the Model
class might not be currently documented at pymc.Model
. If that were the case ignore it; the docs for pymc.Model will be fixed at some point and everything will work again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed this one based on your suggestions.
Optional model for sampling step. Defaults to None (taken from context). | ||
mode: string or `Mode` instance. | ||
mode : string or `Mode` instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know what Mode
is so it would be nice to have it as a link, unfortunately I can't give a suggestion unless I know where it should point to. Hopefully someone else can comment on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a pytensor thing... Not sure we have a more precise signature anywhere. The class is https://github.com/pymc-devs/pytensor/blob/a76172ee6a9753266150fc7bac4dd66906967a26/pytensor/compile/mode.py#L275
But it can also be one of a predefined set of strings :D
Can we merge this? Or do we need to wait for the type hint thing? |
I think we can use
Let's use this and merge, once we agree on pymc-devs/pytensor#690 about the best way to link to that we can add |
Description
First time contributor to open-source and the project for me.
I updated the docstring of BinaryGibbsMetropolis to follow numpydoc format. I also updated the Metropolis init while trying to check how Sphinx works (deleting it would be pointless so I keep it there).
Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pymc--7212.org.readthedocs.build/en/7212/