Skip to content

Add type hints to distribution parameters #6635

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

Merged
merged 5 commits into from
Apr 7, 2023

Conversation

iykat
Copy link
Contributor

@iykat iykat commented Mar 30, 2023

What is this PR about?
...Added type hints to distribution parameters
issue #5358
#DataUmbrellaPyMCSprint
@reshamas @cluhmann @SangamSwadiK

Checklist

Documentation

  • ... Updated some of the typings in the distribution parameter.

📚 Documentation preview 📚: https://pymc--6635.org.readthedocs.build/en/6635/

@reshamas
Copy link
Member

#DataUmbrellaPyMCSprint

@iykat iykat changed the title Distributions parameter Add Type Hints to distribution parameters Mar 30, 2023
@cluhmann
Copy link
Member

This looks good to me. The one omission is alpha=0.0 in the Wald distribution. I suspect that it's also of type Optional[DIST_PARAMETER_TYPES] but default to =1.0 rather than =None.

@iykat
Copy link
Contributor Author

iykat commented Mar 30, 2023

This looks good to me. The one omission is alpha=0.0 in the Wald distribution. I suspect that it's also of type Optional[DIST_PARAMETER_TYPES] but default to =1.0 rather than =None.

Thank you for the feedback. I will make the necessary changes.

@iykat
Copy link
Contributor Author

iykat commented Apr 1, 2023

@cluhmann I have added more type hints to distribution parameters and typed the Wald distribution as you suggested.

Copy link
Member

@cluhmann cluhmann left a comment

Choose a reason for hiding this comment

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

Looks great to me. I am going to approve this to get the workflows working and then we can see where things are at. Someone should sanity check the changes just to make sure I haven't missed anything before merging.

@cluhmann
Copy link
Member

cluhmann commented Apr 3, 2023

I lied (I'm not qualified). Can someone review so we can see how all the CI shakes out?

@reshamas
Copy link
Member

reshamas commented Apr 4, 2023

Note: This PR was submitted using Gitpod!

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #6635 (e92fa8a) into main (b7764dd) will decrease coverage by 2.63%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6635      +/-   ##
==========================================
- Coverage   91.96%   89.33%   -2.63%     
==========================================
  Files          94       94              
  Lines       15927    15927              
==========================================
- Hits        14647    14229     -418     
- Misses       1280     1698     +418     
Impacted Files Coverage Δ
pymc/distributions/continuous.py 97.70% <100.00%> (ø)

... and 16 files with indirect coverage changes

@cluhmann
Copy link
Member

cluhmann commented Apr 6, 2023

If I am reading the log correctly, this seems to be some plotting/Tcl error on windows? Does that have anything to do with this PR?

@ricardoV94
Copy link
Member

ricardoV94 commented Apr 6, 2023

If it's the GP plot test it crops up now and then. No idea why

@cluhmann
Copy link
Member

cluhmann commented Apr 6, 2023

Ah. That seems to be it.

def test_plot_gp_dist_warn_nan(self):

@ricardoV94 ricardoV94 merged commit 7973967 into pymc-devs:main Apr 7, 2023
@welcome
Copy link

welcome bot commented Apr 7, 2023

Congratulations Banner
Congrats on merging your first pull request! 🎉 We here at PyMC are proud of you! 💖 Thank you so much for your contribution 🎁

@iykat iykat changed the title Add Type Hints to distribution parameters Add Type hints to distribution parameters Apr 12, 2023
@iykat iykat changed the title Add Type hints to distribution parameters Add type hints to distribution parameters Apr 12, 2023
@iykat iykat mentioned this pull request Jun 24, 2023
5 tasks
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.

5 participants