✅

Hi, I am the SymPy bot (v167). 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:

• matrices
  • DEPRECATION: Previously the check argument to e.g. MatAdd could be used to form a mathematically incorrect expression like MatAdd(1, Matrix([[1, 2]], check=False). The intention of the check=False argument is just as an optimization to avoid checking that all arguments are matrices with matching shapes for speed. However, it allowed creating objects that were both mathematically invalid and also break the internal invariant of MatAdd which expects that all of its args are of type MatrixExp leading to obscure failures in other routines such as printing. In SymPy 1.11 passing check to MatAdd, MatMul and HadamardProduct will emit a deprecation warning. In a future version of SymPy the arguments will always be checked and passing any non-Matrix argument will give an exception. (#23671 by @oscargus)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.11.

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

For some reason the `check` argument to `MatAdd` is `False` which is inconsistent with `MatMul` and allows creating things like `MatAdd(matrix, scalar)` that is later on not supported.

`MatExpr.as_coeff_mmul` did not return a sympified result (compare to `MatMul.as_coeff_mmul`).

#### Other comments


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

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 -->
* matrices
   * DEPRECATION: Previously the check argument to e.g. `MatAdd` could be used to form a mathematically incorrect expression like `MatAdd(1, Matrix([[1, 2]], check=False)`. The intention of the `check=False` argument is just as an optimization to avoid checking that all arguments are matrices with matching shapes for speed. However, it allowed creating objects that were both mathematically invalid and also break the internal invariant of `MatAdd` which expects that all of its args are of type `MatrixExp` leading to obscure failures in other routines such as printing. In SymPy 1.11 passing `check` to `MatAdd`, `MatMul` and `HadamardProduct` will emit a deprecation warning. In a future version of SymPy the arguments will always be checked and passing any non-Matrix argument will give an exception.
<!-- END RELEASE NOTES -->

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

       before           after         ratio
     [edf24253]       [697220be]
+     25.0±0.08μs      77.8±0.07μs     3.11  matrices.TimeMatrixExpression.time_MatAdd

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [77f1d79c]       [edf24253]
     <sympy-1.10.1^0>                 
+         116±2ms          209±1ms     1.80  sum.TimeSum.time_doit

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

Looks good to me. The release note should clarify that this will lead to an exception in some cases.

I added a deprecation commit.

I think this looks good. The release note should explain the deprecation.

There is also a check parameter to HadamardProduct. I guess that should be deprecated too?

The docs is not rendering perfectly: https://output.circle-artifacts.com/output/job/5ed52a3a-d256-4abf-bc5c-1cc310325118/artifacts/0/doc/_build/html/explanation/active-deprecations.html?highlight=deprecation

But will have to check how this differs to the others.

Will improve the release note as well.

I'm not sure what release note you expect. I can/will of course edit the deprecation section after merging, but before it it merged, it is not so clear what more is expected.

The benchmark change is probably related?

       before           after         ratio
     [b1f70e81]       [01ce91bc]
+     23.1±0.08μs      69.0±0.07μs     2.98  matrices.TimeMatrixExpression.time_MatAdd

However, it is not really realistic to have a benchmark that doesn't check...

I guess the release note just needs the same information that's in the active deprecations file i.e. what is being checked and what specifically is deprecated.

Maybe something like:

• matrices
  • DEPRECATION: Previously the check argument to MatAdd could be used to form a mathematically incorrect sum like MatAdd(1, Matrix([[1, 2]], check=False). The intention of the check=False argument is just as an optimisation to avoid checking that all arguments are matrices for speed. However it allowed creating objects that were both mathematically invalid and also break the internal invariant of MatAdd which expects that all of its args are of type MatrixExpr leading to obscure failures in other routines such as printing. In SymPy 1.11 passing check=False to MatAdd will emit a deprecation warning. In a future version of SymPy the arguments will always be checked and passing any non-Matrix argument will give an exception.

The benchmark change is probably related?

Probably. I don't think that this is an important benchmark but one way to improve it would be to rearrange the logic to make the common path the fast one:

if check:
   # do checking
else:
   # handle False, None, maybe emit a warning etc.

You can see the benchmark here:
https://github.com/sympy/sympy_benchmarks/blob/c9e500f8c7a2c9ed6496103dc7c1278e8a900635/benchmarks/matrices.py#L9-L23

Another possibility is to try to speed up the checking:

I thought something like that should do in the deprecation part, but it is now updated. (Note that the same holds for MatMul and HadamardProduct, so I tried to keep it compact.

Regarding optimization, I do not see any obvious advantage now as it will do away (and there will probably be a different way to create these directly, although maybe not for the benchmarks...).

Looks good to me.