Skip to content

Use more readable ignore logprob #6596

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 2 commits into from
Mar 13, 2023

Conversation

ricardoV94
Copy link
Member

This PR replaces uses of assign_custom_measurable_outputs by the more readable ignore_logprob.

It also refactors a utility to ignore the logprob of multiple (potentially nested) variables consistently.

This PR is a spinoff of #6529

@ricardoV94 ricardoV94 added maintenance logprob no releasenotes Skipped in automatic release notes generation labels Mar 13, 2023
@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #6596 (b67d128) into main (457421b) will decrease coverage by 0.50%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6596      +/-   ##
==========================================
- Coverage   92.03%   91.54%   -0.50%     
==========================================
  Files          92       92              
  Lines       15539    15541       +2     
==========================================
- Hits        14302    14227      -75     
- Misses       1237     1314      +77     
Impacted Files Coverage Δ
pymc/logprob/censoring.py 100.00% <100.00%> (ø)
pymc/logprob/cumsum.py 100.00% <100.00%> (ø)
pymc/logprob/mixture.py 98.32% <100.00%> (-0.03%) ⬇️
pymc/logprob/tensor.py 81.35% <100.00%> (-1.05%) ⬇️
pymc/logprob/transforms.py 96.35% <100.00%> (ø)
pymc/logprob/utils.py 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

@ricardoV94 ricardoV94 requested a review from Armavica March 13, 2023 20:50
Copy link
Member

@Armavica Armavica left a comment

Choose a reason for hiding this comment

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

Is there a reason to not inline assign_custom_measurable_outputs inside ignore_logprob since I believe it is now only used there?

@ricardoV94
Copy link
Member Author

Is there a reason to not inline assign_custom_measurable_outputs inside ignore_logprob since I believe it is now only used there?

That function is a bit more powerful. It can be used to toggle some outputs on and off as measurable.

Although it's not being used anywhere else I would leave it separately until it's proven not ever neeeded

@ricardoV94 ricardoV94 merged commit 9836d00 into pymc-devs:main Mar 13, 2023
Copy link
Member

@larryshamalama larryshamalama left a comment

Choose a reason for hiding this comment

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

This PR has already been merged, but I still had some questions here and there 😅

@@ -294,3 +305,32 @@ def reconsider_logprob(rv: TensorVariable) -> TensorVariable:
new_node.op = copy(new_node.op)
new_node.op.__class__ = original_op_type
return new_node.outputs[node.outputs.index(rv)]


def ignore_logprob_multiple_vars(
Copy link
Member

Choose a reason for hiding this comment

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

Would it be good to add a test for ignore_logprob_multiple_vars?

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 functionality is a core requirement of stack_logprob, and it's covered in this test, when it was first introduced:

@pytest.mark.parametrize("reverse", (False, True))
def test_measurable_make_vector_interdependent(reverse):
"""Test that we can obtain a proper graph when stacked RVs depend on each other"""
x = pt.random.normal(name="x")
y_rvs = []
prev_rv = x
for i in range(3):
next_rv = pt.random.normal(prev_rv + 1, name=f"y{i}")
y_rvs.append(next_rv)
prev_rv = next_rv
if reverse:
y_rvs = y_rvs[::-1]
ys = pt.stack(y_rvs)
ys.name = "ys"
x_vv = x.clone()
ys_vv = ys.clone()
logp = joint_logprob({x: x_vv, ys: ys_vv})
assert_no_rvs(logp)
y0_vv = y_rvs[0].clone()
y1_vv = y_rvs[1].clone()
y2_vv = y_rvs[2].clone()
ref_logp = joint_logprob({x: x_vv, y_rvs[0]: y0_vv, y_rvs[1]: y1_vv, y_rvs[2]: y2_vv})
rng = np.random.default_rng()
x_vv_test = rng.normal()
ys_vv_test = rng.normal(size=3)
np.testing.assert_allclose(
logp.eval({x_vv: x_vv_test, ys_vv: ys_vv_test}),
ref_logp.eval(
{x_vv: x_vv_test, y0_vv: ys_vv_test[0], y1_vv: ys_vv_test[1], y2_vv: ys_vv_test[2]}
),
)

#6342

This PR just refactored it out, because it proved useful elesewhere #6529


Having said that, we can definitely add a targeted test.

@ricardoV94 ricardoV94 changed the title User more readable ignore logprob Use more readable ignore logprob Mar 14, 2023
@ricardoV94 ricardoV94 deleted the refactor_ignore_logprob branch June 6, 2023 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logprob maintenance no releasenotes Skipped in automatic release notes generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants