Implement marginalization as a model transformation and deprecate MarginalModel

[ricardoV94]

Closes #360
Closes #383

Depends on pymc-devs/pymc#7568
Depends on pymc-devs/pymc#7569

[ricardoV94]

The marginalization part is working. Missing the recover_marginals refactor now

[zaxtax]

I have a small concern about the __hash__ and __eq__ methods on MarginalRV

Those can hide bugs that are later hard to find. Do we really need it? Other than that LGTM!

[zaxtax]

I don't like this. Would it be possible to move this logic into equal_computations or a similar place? Also is this actually used anywhere as is?

[ricardoV94]

As the comment mentions this will be moved to the base class in PyTensor. It's used for the equal computations check that make one/two tests much more straightforward to write.

[zaxtax]

Would it be possible to inline into the equal computations check?

…

On Fri, 3 Jan 2025, 12:03 Ricardo Vieira, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pymc_extras/model/marginal/distributions.py <#388 (comment)> : > @@ -43,6 +55,74 @@ def support_axes(self) -> tuple[tuple[int]]: ) return tuple(support_axes_vars) + def __eq__(self, other): + # Just to allow easy testing of equivalent models, + # This can be removed once pymc-devs/pytensor#1114 is fixed + if type(self) is not type(other): + return False + + return equal_computations( + self.inner_outputs, + other.inner_outputs, + self.inner_inputs, + other.inner_inputs, + ) + + def __hash__(self): As the comment mentions this will be moved to the base class in PyTensor. It's used for the equal computations check that make one/two tests much more straightforward to write. — Reply to this email directly, view it on GitHub <#388 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACCUONHAYALMIYWVFFKPL2IZVANAVCNFSM6AAAAABRAGUCLOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRYHEYDCNRVGA> . You are receiving this because your review was requested.Message ID: ***@***.***>

[ricardoV94]

That's mixing concerns. Ops are supposed to be comparable on equality. That also allows cache/merge optimizations elsewhere.

[zaxtax]

We need a better equality check in that case. But this issue isn't a blocker and I'm ok with it better merged as is

…

On Fri, 3 Jan 2025, 15:27 Ricardo Vieira, ***@***.***> wrote: That's mixing concerns. Ops are supposed to be comparable on equality. That also allows cache/merge optimizations elsewhere. — Reply to this email directly, view it on GitHub <#388 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACCUMUYCEIFWSPWHDFQYD2I2M3FAVCNFSM6AAAAABRAGUCLOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRZGI4TOOBSGU> . You are receiving this because your review was requested.Message ID: ***@***.***>

[zaxtax]

LGTM!

[fonnesbeck]

What about the example in pymc-examples? Do we need an issue to update that?

[zaxtax]

Good call!

…

On Sat, 4 Jan 2025, 03:28 Chris Fonnesbeck, ***@***.***> wrote: What about the example in pymc-examples? Do we need an issue to update that? — Reply to this email directly, view it on GitHub <#388 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACCUIEUZ56IEG73FXCMGL2I5BLTAVCNFSM6AAAAABRAGUCLOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZQGAYTCOJWHA> . You are receiving this because your review was requested.Message ID: ***@***.***>

[zaxtax]

Should we do a version bump and release first?

[ricardoV94]

Should we do a version bump and release first?

Yup