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

Add typing to discrete distributions #6410

Conversation

jessica-writes-code
Copy link

What is this PR about?
Per #5358 , it would be nice to have type hints for distribution parameters.

Checklist

Major / Breaking Changes

  • N/A

New features

  • N/A

Bugfixes

  • N/A

Documentation

  • N/A

Maintenance

  • Add typing for distribution parameters.

@canyon289
Copy link
Member

Thanks for contributing to PyMC!

@codecov
Copy link

codecov bot commented Dec 24, 2022

Codecov Report

Merging #6410 (03365b8) into main (1ed4475) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6410      +/-   ##
==========================================
- Coverage   91.99%   91.97%   -0.02%     
==========================================
  Files          94       94              
  Lines       15944    15945       +1     
==========================================
- Hits        14667    14665       -2     
- Misses       1277     1280       +3     
Impacted Files Coverage Δ
pymc/distributions/discrete.py 98.73% <100.00%> (+<0.01%) ⬆️

... and 1 file with indirect coverage changes

@jessica-writes-code
Copy link
Author

It seems like the new typing has introduced a mypy error that didn't exist before in discrete.py, but exists in several other files -- that the signatures for dist in the child classes are incompatible with the parent. I'm not sure there exists a good short-term fix, but I think there are two (probably more) potential long-term fixes:
(1) Adjust signature of dist in the Distribution class. (This will probably require adjusting how it is called in several places.)
(2) Rename the dist method of the Distribution class.
My vote is for (1), but I'm happy to work on either, if it would be useful.

(Also, apologies for not commenting on #5358 before starting this. I'll make sure to do that more effectively going forward.)

@ricardoV94
Copy link
Member

Has this PR been superseded by others?

Not sure who to ping: @OriolAbril ?

@OriolAbril
Copy link
Member

I have no idea about typing, sorry

@ricardoV94
Copy link
Member

Sorry, I thought this PR was part of some sprint

@ricardoV94
Copy link
Member

It seems the type-hints are still missing on main. @jessica-writes-code can you fix the conflicts that have emerged in the PR?

@jessica-writes-code
Copy link
Author

@ricardoV94 Changes made!

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @jessica-writes-code

@jessica-writes-code
Copy link
Author

@ricardoV94 Mypy check is failing. See my comment from Dec 25. Happy to pursue either option or any alternative you suggest!

@ricardoV94
Copy link
Member

I am closing this one as a long time has passed. Type hints may already have been improved in the meantime. Feel free to reopen a PR if type hints are still missing. Thanks for the help!

@ricardoV94 ricardoV94 closed this Aug 24, 2023
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.

4 participants