Refactor Flat and HalfFlat distributions #4723
Conversation
ricardoV94
force-pushed
the
flat_dist
branch
2 times, most recently
from
5ca23ab to
f950baa
Compare
There was a problem hiding this comment.
I still question the purpose/place of these Flat distributions (and, now, RandomVariables); however, this PR looks fine.
At some point, we need to revisit these and determine whether or not they're still needed or relevant given the new framework (or a potentially newer one).
| if testval is None: | ||
| testval = np.full(size, floatX(0.0)) |
There was a problem hiding this comment.
I'm assuming these test values were necessary because, when aesara.config.compute_test_value is enabled, RandomVariable.perform is called in order to generate a test value automatically, and it will fail.
In general, test value-related logic like this needs to be conditioned on aesara.config.compute_test_value != "off"; otherwise, we'll end up doing unnecessary work. This case might be different, though.
There was a problem hiding this comment.
The Flat testval is critical (in the codebase) in a couple of places (e.g., for getting the model.initial_point for sampling).
There was a problem hiding this comment.
Just keep in mind that v4 cannot actually require test values, and anywhere that currently does is a place we need to refactor.
Aside from that, it should always be possible to enable test values and have them work throughout v4, it's just not mandatory or automatically enabled at any point within PyMC.
There was a problem hiding this comment.
Definitely, I am adding a note about this in #4567
* Refactor Flat and HalfFlat distributions * Re-enable Gumbel logp test * Remove redundant test
Replaced
ValueErrorbyNotImplementedError.Also cleaned some tests that had wrong
pytest.marksor were redundant.Depending on what your PR does, here are a few things you might want to address in the description: