Skip to content

Conversation

Costor
Copy link
Contributor

@Costor Costor commented Mar 30, 2023

References to other Issues or PRs

Brief description of what is fixed or changed

TensorProduct doesn't identify ImmutableMatrix (or any other matrix class that is not derived from mutable Matrix) as a matrix:

from sympy import *
from sympy.physics.quantum.tensorproduct import TensorProduct
X = Matrix([[0, 1], [1, 0]])
TensorProduct(X, X)
# displays full 4 x 4 matrix with all 1's on anti-diagonal
Xi = ImmutableMatrix([[0, 1], [1, 0]])
TensorProduct(Xi, Xi)
# displays unevaluated Xi * Xi

The reason is that TensorProduct.__new__() checks if the arguments are of type Matrix instead of MatrixBase. Matrix is mutable, so no immutable matrix type inherits from Matrix, and immutable matrices are not passed on to matrix_tensor_product(). This clearly is a typo, as in sympy.physics.quantum.matrixutils.matrix_tensor_product(), sympy.matrices.kronecker.matrix_kronecker_product() and any other function e.g. in matrixutils.py the code checks for isinstance(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, since scipy_sparse_matrix type is accepted for evaluation in TensorProduct. So in order to avoid further implications and also to exclude symbolic matrices like Identity() which matrix_kronecker_product() cannot handle anyway, I propose to accept ImmutableMatrix in addition to Matrix instead of all MatrixBase.

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

  • physics.quantum
    • TensorProduct now processes immutable matrices and mutable matrices alike. Fixed a bug that TensorProduct didn't compute the result matrix if the argument matrices were immutable.

@sympy-bot
Copy link

sympy-bot commented Mar 30, 2023

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:

  • physics.quantum
    • TensorProduct now processes immutable matrices and mutable matrices alike. Fixed a bug that TensorProduct didn't compute the result matrix if the argument matrices were immutable. (#24993 by @Costor)

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.
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed
`TensorProduct` doesn't identify `ImmutableMatrix` (or any other matrix class that is not derived from mutable `Matrix`) as a matrix:
```
from sympy import *
from sympy.physics.quantum.tensorproduct import TensorProduct
X = Matrix([[0, 1], [1, 0]])
TensorProduct(X, X)
# displays full 4 x 4 matrix with all 1's on anti-diagonal
Xi = ImmutableMatrix([[0, 1], [1, 0]])
TensorProduct(Xi, Xi)
# displays unevaluated Xi * Xi
```
The reason is that `TensorProduct.__new__()`  checks if the arguments are of type `Matrix` instead of `MatrixBase`. `Matrix` is mutable, so no immutable matrix type inherits from `Matrix`, and immutable matrices are not passed on to `matrix_tensor_product()`. This clearly is a typo, as in `sympy.physics.quantum.matrixutils.matrix_tensor_product()`, `sympy.matrices.kronecker.matrix_kronecker_product()` and any other function e.g. in `matrixutils.py` the code checks for `isinstance(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, since `scipy_sparse_matrix` type _is accepted_ for evaluation in TensorProduct. So in order to avoid further implications and also to exclude symbolic matrices like `Identity()` which `matrix_kronecker_product()` cannot handle anyway, I propose to accept `ImmutableMatrix` in addition to `Matrix` instead of all `MatrixBase`.

#### 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

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers. Formerly, `log(-x)` incorrectly gave `-log(x)`.

* physics.units
  * Corrected a semantical error in the conversion between volt and statvolt which
    reported the volt as being larger than the statvolt.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* physics.quantum
  * TensorProduct now processes immutable matrices and mutable matrices alike. Fixed a bug that TensorProduct didn't compute the result matrix if the argument matrices were immutable.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@github-actions
Copy link

github-actions bot commented Mar 30, 2023

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

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
(click on checks at the top of the PR).

@hanspi42
Copy link
Contributor

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.

@Costor
Copy link
Contributor Author

Costor commented Mar 31, 2023

Added a test case for before/after comparison

@hanspi42
Copy link
Contributor

And I have tested the test case. Thank you very much for the contribution.

@hanspi42 hanspi42 merged commit cd9ff53 into sympy:master Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants