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

Discrete uniform and hyper geometric moment #5167

Merged

Conversation

farhanreynaldo
Copy link
Contributor

@farhanreynaldo farhanreynaldo commented Nov 10, 2021

Add moments and tests for the below distributions as part of #5078:

  • pymc.distributions.Discrete.DiscreteUniform
  • pymc.distributions.Discrete.HyperGeometric

@farhanreynaldo
Copy link
Contributor Author

I am not quite sure why the HyperGeometric moment tests are failing, I have checked the calculation manually but it yielded a different result. Is it anything to do with the good and bad variables?

@ricardoV94 ricardoV94 mentioned this pull request Nov 10, 2021
51 tasks
@@ -920,6 +920,12 @@ def dist(cls, N, k, n, *args, **kwargs):
n = at.as_tensor_variable(intX(n))
return super().dist([good, bad, n], *args, **kwargs)

def get_moment(rv, size, N, k, n):
mode = intX(at.floor((n + 1) * (k + 1) / (N + 2)))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mode = intX(at.floor((n + 1) * (k + 1) / (N + 2)))
mode = at.floor((n + 1) * (k + 1) / (N + 2))

Should be fine without intX, we take care of that in the method that calls get_moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I removed it in the latest commit.

@ricardoV94
Copy link
Member

I am not quite sure why the HyperGeometric moment tests are failing, I have checked the calculation manually but it yielded a different result. Is it anything to do with the good and bad variables?

The get_moment should expect whatever is passed to super().dist at the end of the dist classmethod. That means get_moment is getting good, bad, and n. You will have to work with those (or convert back to N, k, n) if you need them.

@codecov
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

Merging #5167 (43585e1) into main (7745f55) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5167      +/-   ##
==========================================
+ Coverage   78.05%   78.10%   +0.04%     
==========================================
  Files          88       88              
  Lines       14142    14153      +11     
==========================================
+ Hits        11039    11054      +15     
+ Misses       3103     3099       -4     
Impacted Files Coverage Δ
pymc/distributions/discrete.py 99.42% <100.00%> (+0.01%) ⬆️
pymc/bart/pgbart.py 96.61% <0.00%> (+0.37%) ⬆️
pymc/backends/report.py 91.60% <0.00%> (+2.09%) ⬆️

@farhanreynaldo
Copy link
Contributor Author

I am not quite sure why the HyperGeometric moment tests are failing, I have checked the calculation manually but it yielded a different result. Is it anything to do with the good and bad variables?

The get_moment should expect whatever is passed to super().dist at the end of the dist classmethod. That means get_moment is getting good, bad, and n. You will have to work with those (or convert back to N, k, n) if you need them.

So whenever we create a get_moment function, the arguments that we passed should match the arguments inside super().dist?

@ricardoV94
Copy link
Member

I am not quite sure why the HyperGeometric moment tests are failing, I have checked the calculation manually but it yielded a different result. Is it anything to do with the good and bad variables?

The get_moment should expect whatever is passed to super().dist at the end of the dist classmethod. That means get_moment is getting good, bad, and n. You will have to work with those (or convert back to N, k, n) if you need them.

So whenever we create a get_moment function, the arguments that we passed should match the arguments inside super().dist?

Yes, that what will be used when we call get_moment internally

@farhanreynaldo
Copy link
Contributor Author

Is the failing test related to this PR?

@ricardoV94
Copy link
Member

Probably not, seems to be related to #4771

@ricardoV94
Copy link
Member

Seems good to me. You have some conflicts that need to be fixed though

@farhanreynaldo farhanreynaldo force-pushed the discrete-uniform-hyper-geometric branch from 50d484c to b0cf4c5 Compare November 10, 2021 12:14
@ricardoV94 ricardoV94 merged commit f24a1df into pymc-devs:main Nov 10, 2021
@ricardoV94
Copy link
Member

Thanks @farhanreynaldo

@farhanreynaldo farhanreynaldo deleted the discrete-uniform-hyper-geometric branch November 12, 2021 02:26
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