Closed brandonwillard closed 1 year ago
Base: 95.08% // Head: 95.15% // Increases project coverage by +0.06%
:tada:
Coverage data is based on head (
369b835
) compared to base (d0c009d
). Patch coverage: 99.08% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
This looks great, and now that it's cleaned up it looks more obvious to me what can be further improved.
Do you want to also go ahead and include the changes in #184 or do you want to leave it for future work?
I am not convinced that in the simple situations AePPL currently supports we need to rewrite the graph at all, although it could be useful to have a "canonicalization" phase where we could rewrite transforms of RandomVariable
when they return a known distribution (ratio of StandardNormalRV
for instance). Here, the model graphs contain almost all the information we need. To be able to compute the model's logdensity without rewriting the graph we would also need:
RandomVariable
's logdensity.However, pushed a little further, rewriting the graph could be very interesting for AeMCMC. We would need to replace every random and observed variable in the graph by their corresponding measure, including their domain and logdensity graph. That means starting from the known measures and pushing them through the transforms, thus getting a graph of Measures; transforms are then irrelevant if we also keep track of the relation between value variables in parallel. This is probably what we should be aiming for in these rewrites, instead of replacing Aesara operators with RVTransform
s.
Do you want to also go ahead and include the changes in #184 or do you want to leave it for future work?
Let's do that one in a follow-up.
More general comment
Yes, there's a lot to rework here. I'm going to merge this one so that—in the meantime—our existing functionality is preserved and better designed. At the very least, it should be a lot easier to rewrite/redesign in this form compared to the current.
This PR refactors the design of
MeasurableElemwise
and the rewrites that use it (i.e. the changes mostly introduced by https://github.com/aesara-devs/aeppl/pull/26).These changes make it easier for others to extend the coverage of measurable
Elemwise
transforms using basic rewrite DB registration. They also put each transformation rewrite under the management of Aesara's rewrite system independently, which means that they can be applied more efficiently (e.g. hash-based lookups on distinctOp
instances) and their application can be enabled/disabled and tracked with more granularity—among other things.The additions in #144 have also been added.
Closes #170