Repository: sympy/sympy
Pull request: DM uses copy
<!-- 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. -->

closes #26973

#### Brief description of what is fixed or changed


#### 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. 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 -->
* matrices
  * domain matrix no longer modifies instantiating list of lists
<!-- END RELEASE NOTES -->


Problem statement:
DomainMatrix will modify lists used for its construction -- intended?
I am not sure that the mutation of the instantiating lists for a DomainMatrix is intended:
```python
>>> l = [1,2]
>>> DomainMatrix([l],(1,2),ZZ)
DomainMatrix([[1, 2]], (1, 2), ZZ)
>>> _[0,0]=42
>>> l
[42, 2]

compare to Matrix

>>> l = [1,2]
>>> Matrix([l])
Matrix([[1, 2]])
>>> _[0,0]=42
>>> l
[1, 2]
```

Hints from issue discussion:
That could be changed in the constructor. I think at the time that I made it I was very concerned about the performance impact of things like copying a list. Some time later and after lots of performance measurements I don't think it would have any significant impact. The `DomainMatrix` constructor is not really used internally anyway.
It might be a nice feature but the fact that it modifies the original should be prominently advertised. The work on `smith_decomp` in #17451 comment would have benefitted from knowing this. Specifically, if `view` is made as a copy of `m[1:,1:]` in the routine, then it will fail, as I recall.
> It might be a nice feature but the fact that it modifies the original should be prominently advertised.

No, it should not. We should not let anyone depend on this because then we would be tied to that forever. We should change this now to copy the lists.

Note that the behaviour here will already be different if python-flint is installed:
```python
In [14]: l = [1, 2]

In [15]: dm = DomainMatrix([l], (1, 2), ZZ)

In [16]: dm
Out[16]: DomainMatrix([[1, 2]], (1, 2), ZZ)

In [17]: dm.rep
Out[17]: DFM([[1, 2]], (1, 2), ZZ)

In [18]: dm.rep.rep
Out[18]: [1, 2]

In [19]: type(dm.rep.rep)
Out[19]: flint.types.fmpz_mat.fmpz_mat

In [20]: dm[0,0]=42
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[20], line 1
----> 1 dm[0,0]=42

File ~/current/active/sympy/sympy/polys/matrices/domainmatrix.py:219, in DomainMatrix.__setitem__(self, key, value)
    217 i, j = key
    218 if not self.domain.of_type(value):
--> 219     raise TypeError
    220 if isinstance(i, int) and isinstance(j, int):
    221     self.rep.setitem(i, j, value)

TypeError: 

In [21]: dm[0,0]=ZZ(42)

In [22]: dm
Out[22]: DomainMatrix([[42, 2]], (1, 2), ZZ)

In [23]: l
Out[23]: [1, 2]
```
> No, it should not.

Documentation (in a docstring) has no bearing on whether something is public or private. If this is intended to be private-can-change-at-will, that's fine. But it should be clearly documented so someone other than the original author knows how to use it in its current state.
> Documentation (in a docstring) has no bearing on whether something is public or private.

It does have a bearing on these things. We should not just randomly document unintended aspects of an implementation as "features" because then we encourage people to use them.

Here is an example:
https://github.com/sympy/sympy/blob/31e957f47a636df9b7e62cd3951ec0afcc266369/sympy/core/numbers.py#L1282-L1290
That was documented by you 233e209 for no reason at all (and naturally self-merged as usual).

That documented statement needs to be broken if we want to e.g. use python-flint or gmpy2 as the internal implementation of Rational. No other implementation of rational numbers supports the notion of an "unevaluated rational" so supporting that makes it impossible to use other implementations.

Why document something if it is a bad idea to allow users to depend on it and if it is something that we will likely want to change in future? You even added unit tests to assure that future implementation would satisfy the broken behaviour.

The fact that an implementation does something now does not make it a feature. In this case what you have identified with DomainMatrix could be described as a bug but definitely not a feature. It was never intended that the implementation would ensure the described behaviour. The expectation when writing the code was that the caller of DomainMatrix relinquishes ownership of the lists they pass in or otherwise it was just overlooked that a copy needed to be made.

There is absolutely no way that I would intended for users to take advantage of DomainMatrix mutating some external lists because that is an absurd design and attempting to satisfy guarantees like that is an absurd constraint that complicates the process of doing actually useful things like improving reasonable use of matrices.

The fix now is definitely not documenting this as a feature: the lists should be copied so that the caller's list are not mutated.
The other issue I find while testing out this is that having `python-flint` installed, even does not allow setting elements by python `int` directly.
```python3
from sympy import *
from sympy.polys.matrices import DomainMatrix

l = [[1, 2]]
M = DomainMatrix(l, (1, 2), ZZ)
M[0, 0] = 3
```
```
File [~/documents/sympy/sympy/polys/matrices/domainmatrix.py:219](http://localhost:8888/home/sylee957/documents/sympy/sympy/polys/matrices/domainmatrix.py#line=218), in DomainMatrix.__setitem__(self, key, value)
    217 i, j = key
    218 if not self.domain.of_type(value):
--> 219     raise TypeError
    220 if isinstance(i, int) and isinstance(j, int):
    221     self.rep.setitem(i, j, value)

TypeError:
```
Using `M[0, 0] = ZZ(3)` works.

I indeed think that mutating the list is bug, than the intended behavior, because using flint backend have different behavior.
Maybe flint uses its own low level implementation of list, rather than getting handle of Python objects directly, so copying list objects are necessary for that. So to get the consistency between python and flint, the old behavior may be considered a bug rather than implicit feature
> The other issue I find while testing out this is that having `python-flint` installed, even does not allow setting elements by python `int` directly.

This is intentional. DomainMatrix is a lower-level type and so while it may have some type checks to prevent errors it does not do things like implicit coercion. We should make a higher-level type that wraps DomainMatrix and is friendlier to use like Poly that uses sympify and has implicit conversion to/from Expr etc.

The problem is once you start allowing something like setting an `int` it becomes a slippery slope where it is hard to define which types should be acceptable and you end up needing to do the equivalent of sympify everywhere which makes everything slow and complicates every public method. Better just to require that the caller use the correct type.

Note that this is already wrong:
```python
dm = DomainMatrix([[1, 2]], (1, 2), ZZ)
```
It should be
```python
dm = DomainMatrix([[ZZ(1), ZZ(2)]], (1, 2), ZZ)
```
There just isn't much error checking here.

There is a friendlier constructor:
```python
In [4]: from sympy.polys.matrices import DM

In [5]: DM([[1, 2], [3, 4]], ZZ)
Out[5]: DomainMatrix([[1, 2], [3, 4]], (2, 2), ZZ)
```
That version can infer the shape and will convert all elements like `ZZ(1)` etc for you.

The best way usually though is just to convert a Matrix like:
```python
In [6]: M = Matrix([[1, 2], [3, 4]])

In [7]: M.to_DM()
Out[7]: DomainMatrix({0: {0: 1, 1: 2}, 1: {0: 3, 1: 4}}, (2, 2), ZZ)
```
Note that this automatically creates a sparse matrix which is often a better choice than dense.

The `__setitem__` case is a bit different. I did not originally want DomainMatrix to be mutable at all. I had to concede some mutating operations just to make it usable as the internal representation of Matrix but I did not want to encourage anyone to use mutating operations with a DomainMatrix. It is typically better just to use a list of lists or some other data structure if you are going to mutate things.
> You even added unit tests to assure that future implementation would satisfy the broken behaviour.

I general add tests defensively so that when they fail that someone will know that the implicit behavior has changed. In this case, the change must be added very carefully after doing an audit of the current use to see if it will be impactful in ways that may not have been tested. The intention of the test is not to cook in a behavior, it is to highlight an assumptions and to bring it to an author's attention.

> because then we encourage people to use them.

If there is code in the codebase it (by definition) can get used. Those using it should know how it behaves. Telling an author how something behaves is not a guarantee that it will not change. Not telling an author how something behaves is a kind of coding alchemy that leaves, as undocumented, features that will need to be understood by someone wanting to use it. It hinders use. 

Issues like mutability are important aspects of how a routine behaves -- I suspect that searching for "*in place*" should yield more than a few hits. Not documenting key behavior is a hinderance to people using the code well. If it is decided that this is to change, then an audit has to be done to make sure that the new behavior does not cause issues. For example, in the code I left for the `smith_decomp` in https://github.com/sympy/sympy/pull/17451 used to assign `m = clear_column(m)`; that is not necessary if the change is happening in-place; it is necessary (and the code will break) if that feature is changed. Maybe a test will catch that. But maybe it won't. But a simple test that asserts that mutability will serve as a caution that the change must be made only after doing a careful review to make sure that there are no hidden logical failures that might not be exhibited as actual failures.

> for no reason at all

I wish you could trust that this is never the case. The comments I left above may give you some insight into the rationale I have for adding comments. And, of course, we know how to handle such flags which are incompatible with some other underlying number representation: raise an error indicating that the use of that flag is not compatible in a given context. (Or, decide that any educator that wants to show an unsimplified fraction must do so by using something like `Symbol('2')/Symbol('4')` instead of `Rational(2,4,gcd=1)`.) Uses of flint numbers where it matters is something that is being handled well even though this `gcd` flag exists and is documented: where it matters, we don't need Flint numbers and when we do want special number representations, we don't need the flag.
> Not telling an author how something behaves is a kind of coding alchemy that leaves, as undocumented, features that will need to be understood by someone wanting to use it. It hinders use.

I don't agree with this. The author here needs to be told not how the behaviour is now but rather what aspects of the behaviour they should be able to depend on and that requires some understanding that some things might change in future. If someone wants to use private/undocumented features then they can but we should not encourage use unless we want something to be used (and therefore not changed in future).

> Uses of flint numbers where it matters is something that is being handled well

We should be using Flint/gmpy2 for Integer/Rational but we are not. I looked into it a few times but each time gave up because there were documented examples that were incompatible with the change.
> I don't agree with this. 

That is fine if you don't agree, but historically SymPy documents how things work, regardless of any author's intentions for the future of code they wrote.  The idea that you state here (we shouldn't document how things work because an author intends to change it in the future or because they didn't have an original intention for it to work that way) is new to me. This seems to presume the original author of the code will be present in the future. In this case, that author is you and you do have the full intent to be present in the future, but we (the general SymPy development team) can't assume that. The appropriate assumption is that any given author will not be present in the future to carry forward any intentions they may have. Every 
line of code
 module/class/function in SymPy is public, whether we like that or not, and the only thing that has made code in SymPy private in the past (with reasonable consistency) is prepending a single underscore to function/class/module names. Public also generally means that all unintended behavior are features, until we deprecate the offending behavior.

> (and naturally self-merged as usual)

I've also noticed this happening much more often these days, but not just from @smichr. As far as I understand, we do not allow merging without sign off from at least one reviewer. It would be nice if we move back to that understanding. Maybe it was lost in the development documentation transfer from the wiki to the sphinx docs (as were many other details about our historical development practices).


There are many SymPy development practices that have been in place for decades now and if we want to change these things (like what makes something private or public OR whether we avoid documenting existing behavior OR whether self-merging is allowed OR whether we allow squashing OR whether we include type hints) need to go through the SYMPEP system to change them. Otherwise we are always in a quagmire of misalignment of how different authors think things should be. Most other major scientific python teams use formal methods to keep everyone aligned (PEPS, periodic video meetings, governance boards, voting, etc.). My opinion is that SymPy also needs these things.

Commit message: DM uses copy

Patch to review:
diff --git a/sympy/polys/matrices/ddm.py b/sympy/polys/matrices/ddm.py
index 2425aaffba64..8f90a3305367 100644
--- a/sympy/polys/matrices/ddm.py
+++ b/sympy/polys/matrices/ddm.py
@@ -117,7 +117,7 @@ def __init__(self, rowslist, shape, domain):
         if len(rowslist) != m or any(len(row) != n for row in rowslist):
             raise DMBadInputError("Inconsistent row-list/shape")
 
-        super().__init__(rowslist)
+        super().__init__([i.copy() for i in rowslist])
         self.shape = (m, n)
         self.rows = m
         self.cols = n
diff --git a/sympy/polys/matrices/tests/test_domainmatrix.py b/sympy/polys/matrices/tests/test_domainmatrix.py
index b7fe91c574ba..ba3e4952ce02 100644
--- a/sympy/polys/matrices/tests/test_domainmatrix.py
+++ b/sympy/polys/matrices/tests/test_domainmatrix.py
@@ -60,6 +60,11 @@ def test_DomainMatrix_init():
 
     raises(DMBadInputError, lambda: DomainMatrix([[ZZ(1), ZZ(2)]], (2, 2), ZZ))
 
+    # uses copy
+    was = [i.copy() for i in lol]
+    A[0,0] = 42
+    assert was == lol
+
 
 def test_DomainMatrix_from_rep():
     ddm = DDM([[ZZ(1), ZZ(2)], [ZZ(3), ZZ(4)]], (2, 2), ZZ)


Code review comments:
diff_hunk: @@ -60,6 +60,11 @@ def test_DomainMatrix_init():
 
     raises(DMBadInputError, lambda: DomainMatrix([[ZZ(1), ZZ(2)]], (2, 2), ZZ))
 
+    # uses copy
+    was = [i.copy() for i in lol]
+    A[0,0] = 42
path: sympy/polys/matrices/tests/test_domainmatrix.py
text: @author:
```suggestion
    A[0,0] = ZZ(42)
```