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 wrong type-hints and stale docstrings from distributions #6280

Merged

Conversation

ricardoV94
Copy link
Member

Related to #4859

This was originally proposed in #6272, see #6272 (review)

Major / Breaking Changes

  • ...

Bugfixes / New features

  • ...

Docs / Maintenance

  • Remove stale logp and logcdf docstrings

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #6280 (d34b557) into main (7608e30) will increase coverage by 0.40%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6280      +/-   ##
==========================================
+ Coverage   94.19%   94.59%   +0.40%     
==========================================
  Files         102      111       +9     
  Lines       21490    25883    +4393     
==========================================
+ Hits        20243    24485    +4242     
- Misses       1247     1398     +151     
Impacted Files Coverage Δ
pymc/distributions/discrete.py 99.64% <ø> (+0.42%) ⬆️
pymc/distributions/continuous.py 97.50% <100.00%> (ø)
pymc/distributions/logprob.py 92.48% <0.00%> (-4.78%) ⬇️
pymc/tests/distributions/util.py 90.00% <0.00%> (-0.87%) ⬇️
pymc/stats/convergence.py 92.72% <0.00%> (-0.21%) ⬇️
pymc/backends/base.py 87.54% <0.00%> (-0.11%) ⬇️
pymc/initial_point.py 100.00% <0.00%> (ø)
pymc/tests/tuning/test_starting.py 100.00% <0.00%> (ø)
pymc/tests/stats/test_convergence.py 100.00% <0.00%> (ø)
pymc/tests/distributions/test_bound.py 100.00% <0.00%> (ø)
... and 31 more

@canyon289
Copy link
Member

Thank you Ricardo! I'll get to this in next 96 hours

@OriolAbril
Copy link
Member

Not blocking the PR, but trying to think forawrd. How do we expect users to know what parameters they need to pass for logp/logcdf to work? several distributions also have alternative parametrizations

@ricardoV94
Copy link
Member Author

ricardoV94 commented Nov 9, 2022

Not blocking the PR, but trying to think forawrd. How do we expect users to know what parameters they need to pass for logp/logcdf to work? several distributions also have alternative parametrizations

Users should never call these methods themselves. The user-facing API is pm.logp(rv, value) For more context see #5308

@canyon289
Copy link
Member

Users should never call these methods themselves. The user-facing API is pm.logp(rv, value) For more context see #5308

Ah this is where I think you and I saw things differently. These are forms of developer documentation for me so if someone is contributing code in the future this is for them. For instance a number of these docstrings would be useful to me right now as a contributor if these methods needed to change. Thats why Im trying to save them (in some form!)

@ricardoV94
Copy link
Member Author

I would bet a newbie developer would be more confused than enlightened by these broken docstrings...

values are desired the values must be provided in a numpy array or Aesara tensor.
mu : tensor_like of float
Mean of the distribution (mu > 0).
lam : tensor_like of float
Copy link
Member

Choose a reason for hiding this comment

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

This is a really good example. I wouldnt know what lam or alpha are by memory and from the logp calculation below theres not enough math to tell me. Having even just a basic description of the parameters would be helpful mathematically.

The other is the what is allowed as inputs. If I was to write code that called this logp I would know what to pass, and auto type checking would be useful.

In the scope of things I care way more about 1 than I do 2, though both would be nice

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a really good example of why I don't like it. The return type is completely wrong, should be TensorVariable (Type) not RandomVariable (Op). Also we only test and promise to support TensorVariable inputs, not float or np.ndarray (even though many times they will work)

Copy link
Member Author

Choose a reason for hiding this comment

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

The parameter information is contained in the main distribution docstring already.

Copy link
Member Author

Choose a reason for hiding this comment

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

mu : tensor_like of float, optional
Mean of the distribution (mu > 0).
lam : tensor_like of float, optional
Relative precision (lam > 0).
phi : tensor_like of float, optional
Alternative shape parameter (phi > 0).
alpha : tensor_like of float, default 0
Shift/location parameter (alpha >= 0).

@maresb maresb closed this Nov 12, 2022
@maresb maresb reopened this Nov 12, 2022
@maresb
Copy link
Contributor

maresb commented Nov 12, 2022

Ok, presumably with the pin in #6282, mypy is now passing. Is this ready to merge? (I have not reviewed.)

@OriolAbril OriolAbril merged commit 5da2617 into pymc-devs:main Nov 15, 2022
@ricardoV94 ricardoV94 deleted the remove_stale_logp_logcdf_docstrings branch June 6, 2023 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants