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

Add pymc support to predictive explorer #450

Merged
merged 5 commits into from
Jun 3, 2024

Conversation

rohanbabbar04
Copy link
Contributor

Description

  • Add pymc support to predictive_explorer.
  • Add parameter engine to predictive_explorer.
  • Add test in test_predictive_explorer.py.
  • 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)
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • Widget states have been properly saved (only for notebooks with widgets) see for details.

preliz/internal/plot_helper.py Outdated Show resolved Hide resolved
preliz/internal/plot_helper.py Outdated Show resolved Hide resolved
preliz/internal/plot_helper.py Outdated Show resolved Hide resolved
preliz/internal/plot_helper.py Outdated Show resolved Hide resolved
@aloctavodia
Copy link
Contributor

sample_prior_predictive emits a message. saying "Samplig", plus a list of the sampled variables. We should disable that message. It is generated with the logging module. We should suppress it locally with a context manager, or disable it before sampling and enable it again after sampling. We need to do this to avoid messing up calls to sample_prior_predictive outside of predictive_explorer

@aloctavodia
Copy link
Contributor

aloctavodia commented Jun 1, 2024

It is not needed for this PR, but something to think about.

We may want to try to guess the value of "engine", for instance for a pymc model we could inspect the source and detect the regex pattern for "with pm.Model() as" where the particle "pm." is something that can vary. For Bambi it should also be simple to detect it.

We may want to keep the "engine" argument and default to "auto" or something similar, just in case our automatic detection fails

@rohanbabbar04
Copy link
Contributor Author

sample_prior_predictive emits a message. saying "Samplig", plus a list of the sampled variables. We should disable that message. It is generated with the logging module. We should suppress it locally with a context manager, or disable it before sampling and enable it again after sampling. We need to do this to avoid messing up calls to sample_prior_predictive outside of predictive_explorer

Created a contextmanager to handle the logs generated during this step.

@rohanbabbar04
Copy link
Contributor Author

It is not needed for this PR, but something to think about.

We may want to try to guess the value of "engine", for instance for a pymc model we could inspect the source and detect the regex pattern for "with pm.Model() as" where the particle "pm." is something that can vary. For Bambi it should also be simple to detect it.

We may want to keep the "engine" argument and default to "auto" or something similar, just in case our automatic detection fails

Sounds good.... Let me create a new issue for this

@rohanbabbar04
Copy link
Contributor Author

rohanbabbar04 commented Jun 2, 2024

I am currently keeping the sampling pymc code separated from the preliz code, like it was discussed in the jupyter notebook.
Better to not disrupt the current implementation of the already existing preliz code.
If required we can combine the codes later...

@aloctavodia
Copy link
Contributor

aloctavodia commented Jun 3, 2024

agreed, we can refactor and combine later. Something really easy to combine is the plotting part, right?

@aloctavodia aloctavodia merged commit e6ea72e into arviz-devs:main Jun 3, 2024
4 checks passed
@rohanbabbar04
Copy link
Contributor Author

agreed, we can refactor and combine later. Something really easy to combine is the plotting part, right?

Yes...

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.

2 participants