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

Adds support for multiple targets with weights to ppe #442

Merged
merged 3 commits into from
May 20, 2024

Conversation

rohanbabbar04
Copy link
Contributor

@rohanbabbar04 rohanbabbar04 commented May 18, 2024

Closes #427

Description

  • Add support for target as a list of tuples with each tuple having PreliZ distribution and weight.
  • Add parameter seed to ppe.
  • Add tests to test_ppe.
  • Update docs.

Checklist

  • Code style is correct (follows pylint and black guidelines)
  • Includes new or updated tests to cover the new feature
  • New features are properly documented (with an example if appropriate)

preliz/internal/optimization.py Outdated Show resolved Hide resolved
preliz/internal/optimization.py Outdated Show resolved Hide resolved
Comment on lines 460 to 463
def get_weighted_rvs(targets, weights, size, seed):
target_rnd_choices = np.random.choice(len(targets), size=size, p=weights)
samples = [target.rvs(size, np.random.default_rng(seed)) for target in targets]
return np.choose(target_rnd_choices, samples)
Copy link
Contributor

Choose a reason for hiding this comment

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

create a random generator once and then use it everywhere. I think it should be generated inside ppe.py and then propagated as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this, I think rng needs to be defined in optimize_pymc_model...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok but the user should be able to pass a seed to ppe

Copy link
Contributor Author

@rohanbabbar04 rohanbabbar04 May 18, 2024

Choose a reason for hiding this comment

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

Hmm...
This sounds good, we can allow the user to input a seed keeping default to be 0.

I'll make the changes accordingly

@aloctavodia aloctavodia merged commit 1c1d007 into arviz-devs:main May 20, 2024
4 checks passed
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.

PPE: Accept more than one distribution as target
2 participants