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

Merge postprocess_fn into the fori_collect loop #1910

Merged
merged 4 commits into from
Nov 20, 2024

Conversation

fehiepsi
Copy link
Member

Test: current tests are passing

# We have two extra calls to the model to get deterministic values:
# 1. transform the init state
# 2. transform state during the loop
assert GLOBAL["count"] == 5 + num_traces_for_heuristic
Copy link
Member

Choose a reason for hiding this comment

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

Could you add an assertion that x_copy is in the get_samples? Perhaps test_mcmc.py would be a better place for the suggested test.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it is tested at

assert samples["logits"].shape == (num_samples, N)

fields = [fields] if len(collect_fields) == 1 else list(fields)
site_values = jax.tree.flatten(fields[0])[0]
if len(site_values) > 0:
fields[0] = postprocess_fn(fields[0], *x[1:])
Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify what is stored in fields[1:] if len(collect_fields) >1 since we only need to run postprocess_fn on the first element?

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 first one is the sample field. The rest are extra fields.

Copy link
Member

Choose a reason for hiding this comment

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

I see, then it makes sense that constraints are only applied to the first field.

Copy link
Member

@OlaRonning OlaRonning left a comment

Choose a reason for hiding this comment

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

Thanks for clarifications

@OlaRonning OlaRonning merged commit 0e7bd20 into pyro-ppl:master Nov 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants