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 initial distribution parameter types #5315

Merged
merged 1 commit into from
Jan 15, 2022
Merged

Conversation

canyon289
Copy link
Member

Per comment #5298 (comment)

Let me know if I got it right @ricardoV94

@codecov
Copy link

codecov bot commented Jan 6, 2022

Codecov Report

Merging #5315 (66deb25) into main (3381019) will decrease coverage by 0.79%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5315      +/-   ##
==========================================
- Coverage   80.23%   79.43%   -0.80%     
==========================================
  Files          89       89              
  Lines       14856    14857       +1     
==========================================
- Hits        11919    11801     -118     
- Misses       2937     3056     +119     
Impacted Files Coverage Δ
pymc/distributions/continuous.py 97.43% <100.00%> (ø)
pymc/distributions/distribution.py 91.40% <100.00%> (+0.03%) ⬆️
pymc/sampling_jax.py 0.00% <0.00%> (-97.48%) ⬇️
pymc/parallel_sampling.py 86.71% <0.00%> (-1.00%) ⬇️

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.

Just to not be mistakenly merged in the meantime

@canyon289 canyon289 force-pushed the update_dist_parameter_types branch from d6f63c4 to d8f5a4d Compare January 8, 2022 23:27
sd: Optional[Union[float, np.ndarray]] = None,
lower: Optional[Union[float, np.ndarray]] = None,
upper: Optional[Union[float, np.ndarray]] = None,
mu: Optional[DIST_PARAMETER_TYPES] = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for previous comment @ricardoV94. Hows it looking now?

One question, are dist parameter always optional? If so I can move the optional flag to the definition

Copy link
Member

Choose a reason for hiding this comment

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

No they are not always optional. Most times they are required actually

Copy link
Member Author

Choose a reason for hiding this comment

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

Hows it look now?

Copy link
Member

Choose a reason for hiding this comment

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

Good!

Copy link
Member Author

Choose a reason for hiding this comment

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

🎉 can we merge? Ill create an issue ticket for data umbrella sprint too have others add to the other parameters.

Thanks Ricardo

@canyon289 canyon289 force-pushed the update_dist_parameter_types branch from d8f5a4d to 66deb25 Compare January 14, 2022 04:10
@canyon289 canyon289 merged commit 38867dd into main Jan 15, 2022
@canyon289 canyon289 deleted the update_dist_parameter_types branch January 17, 2022 03:10
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.

2 participants