Skip to content

Change initial_point default strategy from "prior" to "moment" #5140

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

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Nov 5, 2021

This PR adds a check that we have moments for all distributions and changes the default initial point strategy from "prior" to "moment"

Also added a check in the default test moments that the logp of the provided moment is finite.

Closes #5009
Related to #5078

We also need to decide whether we default to a draw from the prior if a distribution does not have a get_moment implemented or if we just fail in that case (which is what is happening now). It would be trivial to allow this with a try/except block

@ricardoV94 ricardoV94 added this to the v4.0.0-beta1 (vNext) milestone Nov 5, 2021
@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from 3fb0e1a to 88cce65 Compare November 5, 2021 10:56
@codecov
Copy link

codecov bot commented Nov 5, 2021

Codecov Report

Merging #5140 (c6e11eb) into main (0c90e82) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5140      +/-   ##
==========================================
+ Coverage   79.15%   79.17%   +0.01%     
==========================================
  Files          88       88              
  Lines       14311    14316       +5     
==========================================
+ Hits        11328    11334       +6     
+ Misses       2983     2982       -1     
Impacted Files Coverage Δ
pymc/distributions/continuous.py 97.37% <ø> (+0.50%) ⬆️
pymc/initial_point.py 100.00% <100.00%> (ø)
pymc/backends/report.py 89.51% <0.00%> (-2.10%) ⬇️
pymc/parallel_sampling.py 86.33% <0.00%> (-1.00%) ⬇️
pymc/distributions/discrete.py 98.72% <0.00%> (+0.25%) ⬆️
pymc/distributions/distribution.py 95.23% <0.00%> (+0.52%) ⬆️

@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from 88cce65 to d17d0b2 Compare November 17, 2021 10:06
@ricardoV94 ricardoV94 changed the title Add test_all_distributions_have_moments Add test to ensure all distributions have moments Nov 18, 2021
@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from d17d0b2 to b8baccf Compare November 19, 2021 11:15
@ricardoV94 ricardoV94 changed the title Add test to ensure all distributions have moments Change initial_point default strategy from "prior" to "moment" Nov 19, 2021
@ricardoV94 ricardoV94 mentioned this pull request Nov 19, 2021
@ricardoV94
Copy link
Member Author

Getting some weird parallel sampling test failures only on Windows. Perhaps related to #4904?

@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch 2 times, most recently from a1fb23e to b97a157 Compare November 19, 2021 13:12
@ricardoV94
Copy link
Member Author

I am inclined to defaulting back to a prior sample if a distribution does not have a moment, perhaps with a UserWarning like

Moment not defined for distribution X, defaulting to a draw from the prior. This can lead to difficulties during tuning. You can manually define an initval or implement a get_moment dispatched function for this distribution.

This would allow users to more gradually implement their own distributions. We still have a check added in this PR that all our standard distributions have moments implemented.

@michaelosthege
Copy link
Member

I like the fallback plus warning idea. Maybe include a link to the relevant docs section on implementing custom distributions?

@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch 2 times, most recently from 895b44c to 15e3632 Compare November 25, 2021 14:57
@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from 15e3632 to efe1b88 Compare December 6, 2021 18:10
@ricardoV94 ricardoV94 marked this pull request as ready for review December 6, 2021 18:10
@ricardoV94
Copy link
Member Author

We have very few moments missing, and it was consensus that this should no longer block the beta release.

I have added the known missing moments to test_all_distributions_have_moments. This test will fail whenever those moments get implemented, making sure our coverage stays up to date.

@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from efe1b88 to c6268c5 Compare December 6, 2021 19:17
@ricardoV94
Copy link
Member Author

Still have those surprising failing tests on Windows alone...

@michaelosthege
Copy link
Member

Still have those surprising failing tests on Windows alone...

To me it looks like the same problem as #4904 - broadcasts are not correctly propagated through Cast operations.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Dec 8, 2021

@ricardoV94
Copy link
Member Author

Tests are passing with the latest Aesara version

@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from dc47cf1 to c6e11eb Compare December 13, 2021 08:45
@ricardoV94 ricardoV94 merged commit e5fb0e7 into pymc-devs:main Dec 13, 2021
@ricardoV94 ricardoV94 deleted the check_distributions_moments_coverage branch January 3, 2022 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change default initial_point method to "moment"
2 participants