Updated pymc.simulator docstring (typos, defaults, type description) #6035
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6035 +/- ##
==========================================
- Coverage 89.30% 89.29% -0.01%
==========================================
Files 72 72
Lines 12890 12890
==========================================
- Hits 11511 11510 -1
- Misses 1379 1380 +1
|
There was a problem hiding this comment.
Thanks!
It already looks great, and the docs look better and are more clear already. I left some more comments, plus this one here because it is on a line I wasn't able to comment on: the short description also has
``pm.sample_smc``
# should instead be
:func:`~pymc.sample_smc`
pymc/distributions/simulator.py
Outdated
| it should return a 1d numpy array (or Aesara vector). | ||
| epsilon : float or array | ||
| epsilon : float or array, default 1.0 |
There was a problem hiding this comment.
I think this should be tensor_like of float
pymc/distributions/simulator.py
Outdated
| Number of dimensions of the SimulatorRV (0 for scalar, 1 for vector, etc.) | ||
| Defaults to ``0``. | ||
| ndims_params : list[int] | ||
| ndims_params : list[int], default 0 for each parameter. |
There was a problem hiding this comment.
| ndims_params : list[int], default 0 for each parameter. | |
| ndims_params : list of int, optional |
As the default is a bit complex here, it is not a value we can add here but something that depends on other parameters, I would use optional and keep the explanation in the description
pymc/distributions/simulator.py
Outdated
| be passed by order, in which case the keyword argument ``params`` is | ||
| ignored. Alternatively, each parameter can be passed by order after fn, | ||
| ``param1, param2, ..., paramN`` |
There was a problem hiding this comment.
Can't comment further up. It looks like unnamed_params is missing. I think it should be something like
*unnamed_params : list of TensorVariable
description starts here
params : list of TensorVariable
description extended here, plus mention that if both are present an error is thrown.
someone else will need to take over on the actual description though as I don't really know what to write. Is one preferred over the other, are there any potential behaviour differences?
If they are both exactly the same, then the we can follow for example xarray's pattern where this situation is very common with kwargs, see for example: https://docs.xarray.dev/en/stable/generated/xarray.Dataset.rename.html
There was a problem hiding this comment.
It looks like they should behave identically. If the keyword argument isn't passed, then params just gets defined as the unnamed_params and then params is used throughout the rest of the file. I went ahead and wrote a description similar to xarray.
|
Hi Oriol, thanks for the feedback - these suggestions are really helpful. I'm a bit unsure about the procedure. To revise a pull request, do I make the changes on my branch and then resubmit the whole PR through the git command line instructions or is there a different procedure? |
|
Make the changes locally and commit them to the branch you opened your PR from, This is the last section in https://pymc-data-umbrella.xyz/en/latest/sprint/tutorials/pr_tutorial.html#get-ready-for-reviews which has the specific commands and extends the overview I gave in the above paragraph. |
|
I implemented the suggested changes and then pushed them. Also, one thing came up that wasn't strictly a docstring fix. I noticed the file uses inconsistent spellings of Kullback-Leiber and the class is incorrectly called KullbackLiebler. I thought we might want to change all of them to the correct spelling. I searched through the rest of the pymc package and couldn't find any references to the incorrectly spelled class, so I don't think this should cause trouble for other functions. But I'm also happy to ignore this and just stick to docstrings for now. |
|
@daniel-saunders-phil |
|
I’m not sure how to handle this pre-commit problem. I’m gonna try to figure it out for a little bit but I might have some questions in a couple of days. |
|
@daniel-saunders-phil I am working on my 3rd notebook and, once again(!), stuck at the pre-commit stage. (+ possible git issue) @OriolAbril cc: @symeneses |
|
@daniel-saunders-phil if you click on the "Details" button of the checks seciton you'll be able to see the checks logs. In this PR, the issue is with the "trim trailing whitespace": https://github.com/pymc-devs/pymc/runs/7715083643?check_suite_focus=true#step:4:64. That means that there are lines that end up with a white character instead of finishing with the last letter/punctiation sign of the line. If you have followed the steps from https://pymc-data-umbrella.xyz/en/latest/sprint/tutorials/environment_setup.html to set up your environment, you can run |
|
It looks like all tests are passing this time! after a little bit of tinkering, I managed to get pre-commit working. Thanks a lot, @OriolAbril and @reshamas ! |
There was a problem hiding this comment.
Sorry for requesting changes this late, but I think we should keep an alias of the KullbackLeibler class:
class KullbackLiebler(KullbackLeibler):
def __init__(self, *args, **kwargs):
"""Alias for KullbackLeibler."""
warnings.warn("You are using `KullbackLiebler` which is a typo. Use `KullbackLeibler` instead.", DeprecationWarning)
super(). __init__(*args, **kwargs)
|
I considered it too but but eventually decided on not doing it because 1) it is not included in our docs and therefore not part of the public api and 2) no tests broke because it has not been refactored to v4, so it would be a breaking change on the private api with a major change version, from 3.11 to 4.x (whatever the version it gets updated). We can still add the alias to be nice, but then it should have a different type of warning. This is the description of deprecationwarning in https://docs.python.org/3/library/exceptions.html#DeprecationWarning :
|
|
Congrats @daniel-saunders-phil ! #DataUmbrellaPyMCSprint |
What is this PR about?
fixing a number of issues with the pymc.simulator docstring.
...
Checklist
Major / Breaking Changes
Bugfixes / New features
Docs / Maintenance