Fix failed test related to PR #443 by replacing pt.batched_dot with pt.vectorize(pt.dot,...)
#453
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Moved the import statement for blackjax to ensure it is only imported when needed. * Moved blackjax import statement prevents import errors for users on Windows. * Updated the fit function to specify the return type as az.InferenceData.
…nsum * Modified the calculation of in and to use instead of the deprecated function. * Added calculations for in to improve readability. * Renamed to in pathfinder functions to reflect API changes. * Updated warning filters to ignore UserWarnings related to einsum subscripts.
|
It looks like it still has some errors. Once this passes CI we can merge |
|
Ahh damn I spoke too soon. The failures are mostly due to I don't get this error when I run pytest locally (with the same package dependencies). If there's a better way to run tests locally that'll pick up on CI errors, please do share, as I'm keen to know if I'm missing anything! |
|
Make sure you are using the latest pytensor or whichever version CI is on.
If CI is using an old version, we can update it
…On Thu, 10 Apr 2025, 17:52 Michael Cao, ***@***.***> wrote:
Ahh damn I spoke too soon. The failures are mostly due to pytensor.graph.utils.InconsistencyError:
Multiple destroyers of g, which I will look into sometime tomorrow.
I don't get this error when I run pytest locally (with the same package
dependencies). If there's a better way to run tests locally that'll pick up
on CI errors, please do share, as I'm keen to know if I'm missing anything!
—
Reply to this email directly, view it on GitHub
<#453 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUNI2RMU7VXOLXFPGF32Y2HSZAVCNFSM6AAAAAB23Z7U6GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJUGMZDMNJVG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
*aphc14* left a comment (pymc-devs/pymc-extras#453)
<#453 (comment)>
Ahh damn I spoke too soon. The failures are mostly due to pytensor.graph.utils.InconsistencyError:
Multiple destroyers of g, which I will look into sometime tomorrow.
I don't get this error when I run pytest locally (with the same package
dependencies). If there's a better way to run tests locally that'll pick up
on CI errors, please do share, as I'm keen to know if I'm missing anything!
—
Reply to this email directly, view it on GitHub
<#453 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUNI2RMU7VXOLXFPGF32Y2HSZAVCNFSM6AAAAAB23Z7U6GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJUGMZDMNJVG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@aphc14 it might make sense while we figure out what caused this to have a more modest change where we filter the warning for The regression appears to have been introduced between pytensor-2.30.2 pytensor-2.30.3. So we should try to make a minimal failing example and open an issue there too! |
|
The warning is there for performance reasons it means you're using the same letter for something that has a different shape (but broadcasts fine) which is less efficient than using a different letter. Also users will still see the warning even if you ignore in the tests which is not a great experience. |
|
Thanks @zaxtax @ricardoV94 , that narrows it down quite a fair bit. Will attempt to resolve this within the next couple of hours 🤞 |
…e(pt.dot,...) * Fixed errors with deprecated einsum usage in bfgs_sample_dense and bfgs_sample_sparse functions by implementing pt.vectorize(pt.dot,...). * Updated test_pathfinder to filter out deprecation warnings related to JAXopt.
aphc14
changed the title
pt.batched_dot with pt.einsumpt.batched_dot with pt.vectorize(pt.dot,...)
|
@zaxtax @ricardoV94 done :) replaced: with:
|
|
Good stuff! I think we should still try to find a minimal example that
triggers the previous error to file an issue on the pytensor repo
…On Sat, 12 Apr 2025, 18:57 Michael Cao, ***@***.***> wrote:
@zaxtax <https://github.com/zaxtax> @ricardoV94
<https://github.com/ricardoV94> done :)
replaced:
mu = x - pt.einsum("ijk,ik->ij", H_inv, g) # causes error: Multiple destroyers of g
with:
batched_dot = pt.vectorize(pt.dot, signature="(ijk),(ilk)->(ij)")
mu = x - batched_dot(H_inv, pt.matrix_transpose(g[..., None]))
pt.einsum is fussier for some reason, even though pt.vectorize(pt.dot,...)
and the previous pt.batched_dot work
—
Reply to this email directly, view it on GitHub
<#453 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUOBLGMB3C4T2HDGXGD2ZFAWXAVCNFSM6AAAAAB23Z7U6GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJYHEYDKOJZGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
*aphc14* left a comment (pymc-devs/pymc-extras#453)
<#453 (comment)>
@zaxtax <https://github.com/zaxtax> @ricardoV94
<https://github.com/ricardoV94> done :)
replaced:
mu = x - pt.einsum("ijk,ik->ij", H_inv, g) # causes error: Multiple destroyers of g
with:
batched_dot = pt.vectorize(pt.dot, signature="(ijk),(ilk)->(ij)")
mu = x - batched_dot(H_inv, pt.matrix_transpose(g[..., None]))
pt.einsum is fussier for some reason, even though pt.vectorize(pt.dot,...)
and the previous pt.batched_dot work
—
Reply to this email directly, view it on GitHub
<#453 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUOBLGMB3C4T2HDGXGD2ZFAWXAVCNFSM6AAAAAB23Z7U6GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJYHEYDKOJZGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
zaxtax
approved these changes
Apr 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix failed test from PR #443 and replace
pt.batched_dotwithpt.einsummuinbfgs_sample_denseandbfgs_sample_sparseto useeinsuminstead of the deprecatedbatched_dotfunction.H_invinbfgs_sample_sparseto improve readability.compile_pymctocompilein pathfinder functions to reflect API changes.