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

WIP: V4 update test framework for distributions random method #4580

Conversation

matteo-pallini
Copy link
Contributor

@matteo-pallini matteo-pallini commented Mar 28, 2021

The recent distributions refactoring (#4508 and #4548) moves the logic to
get a random variable from a distribution to aesara.
This relies on numpy and scipy random variables implementation.
Given this change the main thing worth testing on the PyMC side is that
the PyMC parametrization is sensible given the one on the Aesara side
(effectively the numpy/scipy one)

More details can be found on issue #4554

  • what are the (breaking) changes that this PR makes? No breaking changes, only fixing currently broken tests
  • are the changes—especially new features—covered by tests and docstrings? This change is only affecting tests
  • right before it's ready to merge, mention the PR in the RELEASE-NOTES.md
  • Is there any other test not relevant anymore given the regactoring?

@brandonwillard brandonwillard marked this pull request as draft March 28, 2021 19:05
@matteo-pallini matteo-pallini force-pushed the v4-update-test-framework-for-distributions-random-method branch from 74b55af to a1b4299 Compare April 1, 2021 23:38
@matteo-pallini matteo-pallini changed the base branch from v4 to master April 1, 2021 23:45
@matteo-pallini matteo-pallini changed the base branch from master to v4 April 1, 2021 23:46
brandonwillard and others added 22 commits April 3, 2021 10:51
…d basic dists

These changes can be summarized as follows:
- `Model` objects now track fully functional Theano graphs that represent all
relationships between random and "deterministic" variables.  These graphs are
called these "sample-space" graphs.  `Model.unobserved_RVs`, `Model.basic_RVs`,
`Model.free_RVs`, and `Model.observed_RVs` contain these
graphs (i.e. `TensorVariable`s), which are generated by `RandomVariable` `Op`s.
- For each random variable, there is now a corresponding "measure-space"
variable (i.e. a `TensorVariable` that corresponds to said variable in a
log-likelihood graph).  These variables are available as `rv_var.tag.value_var`,
for each random variable `rv_var`, or via `Model.vars`.
- Log-likelihood (i.e. measure-space) graphs are now created for individual
random variables by way of the generic functions `logpt`, `logcdf`,
`logp_nojac`, and `logpt_sum` in `pymc3.distributions`.
- Numerous uses of concrete shape information stemming from `Model`
objects (e.g. `Model.size`) have been removed/refactored.
- Use of `FreeRV`, `ObservedRV`, `MultiObservedRV`, and `TransformedRV` has been
deprecated.  The information previously stored in these classes is now tracked
using `TensorVariable.tag`, and log-likelihoods are generated using the
aforementioned `log*` generic functions.
This commit changes `DictToArrayBijection` so that it returns a `RaveledVars`
datatype that contains the original raveled and concatenated vector along with
the information needed to revert it back to dictionay/variables form.

Simply put, the variables-to-single-vector mapping steps have been pushed away
from the model object and its symbolic terms and closer to the (sampling)
processes that produce and work with `ndarray` values for said terms.  In doing
so, we can operate under fewer unnecessarily strong assumptions (e.g. that the
shapes of each term are static and equal to the initial test points), and let
the sampling processes that require vector-only steps deal with any changes in
the mappings.
Classes and functions removed:
- PyMC3Variable
- ObservedRV
- FreeRV
- MultiObservedRV
- TransformedRV
- ArrayOrdering
- VarMap
- DataMap
- _DrawValuesContext
- _DrawValuesContextBlocker
- is_fast_drawable
- _compile_theano_function
- vectorize_theano_function
- get_vectorize_signature
- _draw_value
- draw_values
- generate_samples
- fast_sample_posterior_predictive

Modules removed:
- pymc3.distributions.posterior_predictive
- pymc3.tests.test_random
This make `aesara.graph.basic.clone_replace` work correctly when `Scan`s are
included in a graph.
brandonwillard and others added 22 commits April 3, 2021 10:55
The distributions refactoring moves the random variable sampling to
aesara. This relies on numpy and scipy random variables implementation.
So, now the only thing we care about testing is that the parametrization
on the PyMC side is sendible given the one on the Aesara side
(effectively the numpy/scipy one)

More details can be found on issue pymc-devs#4554
pymc-devs#4554
More details on commit id 0773620b6f599423315035b97ef082ad32d98fd4
More details on commit id 0773620b6f599423315035b97ef082ad32d98fd4
comment.
Most of the random variable logic has been moved to aesara, as well as
most of the relative tests. More details can be found on issue pymc-devs#4554
@matteo-pallini
Copy link
Contributor Author

matteo-pallini commented Apr 3, 2021

Closing this PR because I made a mess while rebasing on v4. I think that this was due to me starting to work from an outdated version of v4 and somehow messing up the branch while rebasing on the up-to-date v4.

Although falling to fix the branch is quite embarrassing, I already spent 2+ hours trying to do so. I am not smart enough to fix it (with reasonable time investment), but smart enough to accept that probably it's not worth spending more time trying to do so.

New PR: #4608

@ricardoV94
Copy link
Member

Although falling to fix the branch is quite embarrassing, I already spent 2+ hours trying to do so. I am not smart enough to fix it (with reasonable time investment), but smart enough to accept that probably it's not worth spending more time trying to do so.

No need to be embarrassed :) I can easily imagine the same happening to me.

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.

5 participants