Skip to content

Adding Interpolated moment #5222

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Dec 9, 2021
Merged

Adding Interpolated moment #5222

merged 8 commits into from
Dec 9, 2021

Conversation

larryshamalama
Copy link
Member

@larryshamalama larryshamalama commented Nov 23, 2021 •
edited
Loading

This PR adds moment and a test for pm.Interpolated (see issue #5078). However, this PR requires issue #5048 to be addressed first (see corresponding WIP PR #5067).

Currently, the tests yield the following error:

ValueError: Must specify bound_args_indices for Interpolated bounded distribution

This PR serves as a placeholder to track my progress.

@codecov
Copy link

codecov bot commented Nov 23, 2021 •
edited
Loading

Codecov Report

Merging #5222 (b50b991) into main (7191e61) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5222   +/-   ##
=======================================
  Coverage   78.98%   78.98%           
=======================================
  Files          88       88           
  Lines       14231    14236    +5     
=======================================
+ Hits        11240    11245    +5     
  Misses       2991     2991
Impacted Files Coverage Δ
pymc/distributions/continuous.py 96.74% <100.00%> (+0.01%) ⬆️
pymc/sampling_jax.py 0.00% <0.00%> (ø)

larryshamalama
larryshamalama commented Nov 24, 2021
View reviewed changes
@ricardoV94 ricardoV94 mentioned this pull request Nov 25, 2021
Closed
51 tasks
@michaelosthege michaelosthege added this to the v4.0.0-beta1 (vNext) milestone Nov 26, 2021
@ricardoV94
Copy link
Member

@larryshamalama Can you try to rebase your PR on the latest version of main to see if the tests pass now that #5067 was merged?

@larryshamalama
Copy link
Member Author

I will do this today!

@ricardoV94
Copy link
Member

ricardoV94 commented Dec 1, 2021 •
edited
Loading

Seems like you are giving invalid values for the spline algorithm that is used inside the Interpolated: https://github.com/pymc-devs/pymc/runs/4374823157?check_suite_focus=true#step:7:742

@larryshamalama
Copy link
Member Author

I fixed the increasing x issue, I just needed values in x_points to be increasing.

However, I am encountering another issue. I don't have full understanding of the dist method but I am getting the following error:

Screen Shot 2021-12-03 at 4 10 01 PM

rv_inputs is something like [x_points, pdf_points, cdf_points] although only x_points and pdf_points are given as argument in the distribution. Is this related to the signature of the dist method of Interpolated?

Reference: https://github.com/pymc-devs/pymc/blob/main/pymc/distributions/continuous.py#L3687

@ricardoV94
Copy link
Member

For conveniente the interpolated RV actually uses 3 variables, x, pdf, cdf, but we create the last for the user in .dist. We could do with just 2 but this just proved easier.

Anyway the get_moment should expect those 3 in the signature even if it doesn't make use of them.

larryshamalama
larryshamalama commented Dec 4, 2021
View reviewed changes
pymc/tests/test_distributions_moments.py Outdated
[
(np.array([-1, 1]), np.array([0.4, 0.6]), None, 0.2),
(np.array([-4, -1, 3, 9, 19]), np.array([0.1 , 0.15, 0.2 , 0.25, 0.3 ]), None, 8),
# (np.array([-22, -4, 0, 8, 13]), np.tile(1 / 5, 5), (5, 3), -np.ones((5, 3))),
Copy link
Member Author

Choose a reason for hiding this comment

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

The result from the moment test is always 1.54589372 despite their dot product being 8. Any thoughts on why I am getting the same, constant error? This may be related to the error that I am encountering in StickBreakingWeights

Copy link
Member Author

Choose a reason for hiding this comment

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

Solved, the answer to this is that Interpolated "connects" the $$(x, f(x))$$ through interpolation. As such, we need to divide the pdf values by Z.

@larryshamalama larryshamalama marked this pull request as ready for review December 8, 2021 19:23
@larryshamalama
Copy link
Member Author

larryshamalama commented Dec 8, 2021 •
edited
Loading

Given that the pdf is normalized, we do not need that sum(pdf_points) equals 1. Should I add another test that has sum(pdf_points) != 1?

ricardoV94
ricardoV94 approved these changes Dec 8, 2021
View reviewed changes
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 great, thanks @larryshamalama

ricardoV94
ricardoV94 requested changes Dec 8, 2021
View reviewed changes
@larryshamalama @ricardoV94
Removed commented test line
b50b991 
Co-authored-by: Ricardo Vieira <[email protected]>
@ricardoV94 ricardoV94 merged commit 45fd9c8 into pymc-devs:main Dec 9, 2021
@larryshamalama
Copy link
Member Author

Thanks to you @ricardoV94 for all your help as always

@larryshamalama larryshamalama deleted the interpolated_moment branch December 9, 2021 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ricardoV94 ricardoV94 ricardoV94 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v4.0.0b2
Development

Successfully merging this pull request may close these issues.
3 participants
@larryshamalama @ricardoV94 @michaelosthege