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

Fix bug in logprob_dimshuffle #181

Merged
merged 3 commits into from Sep 19, 2022

Conversation

ricardoV94
Copy link
Contributor

@ricardoV94 ricardoV94 commented Sep 15, 2022

The logic to unshuffle the dimensions was wrong, but none of the pre-existing tests would reveal it.

The new test condition fails without the fix in multivariate test, and would also have failed in the univariate one, if local_dimshuffle_rv_lift was disabled.

Is there a way to disable the local_dimshuffle_rv_lift rewrite for the purposes of the test?

@ricardoV94 ricardoV94 added the bug Something isn't working label Sep 15, 2022
@brandonwillard
Copy link
Member

brandonwillard commented Sep 15, 2022

Is there a way to disable the local_dimshuffle_rv_lift rewrite for the purposes of the test?

Yeah, we just need to create and use a Mode object with the desired rewrites (e.g. Mode(optimizer=...)). From there, Mode.excluding can be used to remove specific named rewrites.

@ricardoV94
Copy link
Contributor Author

Is there a way to disable the local_dimshuffle_rv_lift rewrite for the purposes of the test?

Yeah, we just need to create and use a Mode object with the desired rewrites (e.g. Mode(optimizer=...)). From there, Mode.excluding can be used to remove specific named rewrites.

But right now there's no way of passing this to factorized_joint_logprob right?

@brandonwillard
Copy link
Member

But right now there's no way of passing this to factorized_joint_logprob right?

Ah, no, but you can add some keyword options to construct_ir_fgraph for that.

@brandonwillard brandonwillard added the op-probability Involves the implementation of log-probabilities for Aesara `Op`s label Sep 15, 2022
@ricardoV94
Copy link
Contributor Author

But right now there's no way of passing this to factorized_joint_logprob right?

Ah, no, but you can add some keyword options to construct_ir_fgraph for that.

Added an option

@ricardoV94
Copy link
Contributor Author

ricardoV94 commented Sep 16, 2022

This looks like a new Aesara-related bug: https://github.com/aesara-devs/aeppl/actions/runs/3067496963/jobs/4953853364

Opened PR to fix it: aesara-devs/aesara#1190

@brandonwillard brandonwillard merged commit b4304fa into aesara-devs:main Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working op-probability Involves the implementation of log-probabilities for Aesara `Op`s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants