-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
TensorProduct fails for ImmutableMatrix: correct to MatrixBase instead of Matrix #24993
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
Conversation
✅ Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.13. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[8bd31f66] [ad253b77]
<sympy-1.12rc1^0>
- 82.6±0.9ms 53.4±0.3ms 0.65 integrate.TimeIntegrationRisch02.time_doit(10)
- 81.5±1ms 52.7±0.6ms 0.65 integrate.TimeIntegrationRisch02.time_doit_risch(10)
Full benchmark results can be found as artifacts in GitHub Actions |
This looks reasonable, but can you add a test case that fails before the change and succeeds after? Otherwise we will not know it if anybody reverts this later. |
Added a test case for before/after comparison |
And I have tested the test case. Thank you very much for the contribution. |
References to other Issues or PRs
Brief description of what is fixed or changed
TensorProduct
doesn't identifyImmutableMatrix
(or any other matrix class that is not derived from mutableMatrix
) as a matrix:The reason is that
TensorProduct.__new__()
checks if the arguments are of typeMatrix
instead ofMatrixBase
.Matrix
is mutable, so no immutable matrix type inherits fromMatrix
, and immutable matrices are not passed on tomatrix_tensor_product()
. This clearly is a typo, as insympy.physics.quantum.matrixutils.matrix_tensor_product()
,sympy.matrices.kronecker.matrix_kronecker_product()
and any other function e.g. inmatrixutils.py
the code checks forisinstance(m, MatrixBase)
, so they work for mutable and immutable matrices alike.However, in the testfile test_tensorproduct.py:25 the original author explicitly tests that
SparseMatrix
should not evaluate - for unknown reason, sincescipy_sparse_matrix
type is accepted for evaluation in TensorProduct. So in order to avoid further implications and also to exclude symbolic matrices likeIdentity()
whichmatrix_kronecker_product()
cannot handle anyway, I propose to acceptImmutableMatrix
in addition toMatrix
instead of allMatrixBase
.Other comments
It's important to use ImmutableMatrix (or MatrixExpr in general) in sympy instead of mutable matrices because the sympy cache cannot work with mutable expressions and this may lead to random errors.
Release Notes