opened
Pristine-Cat
username_0
#### References to other Issues or PRs
squashed commits from #18313

#### Brief description of what is fixed or changed
The main purpose of this PR is to extract the implementations of the core matrix determinant related functions from `MatrixDeterminant` in `matrices/matrices.py` into a separate source file `matrices/determinant.py` as standalone functions. This is based on discussions in #18313 and #18253 and on an expressed desire by members of the SymPy community I have interacted with. Supporting documentation and test changes are also included in this PR. The reasons for this change are all forward-looking:

* The main reason is there is talk of separating the mutable matrix classes completely from the immutable ones. There is even talk of doing a new matrix class system alongside the old one due to many backwards incompatible changes which are desired and could not easily be implemented in the current classes. Either of these changes will be much easier to implement if the underlying concrete functions are available separately for use by the modified or new classes instead of having to copy them from the current classes into each new class.

* There is a desire to eventually change all internal matrix calculations to immutable which would help wich caching (since there is none for Matrices right now). The move to all matrix immutable classes for internal calculations is made difficult by the fact that both the mutable and immutable classes share the same entry point into their respective methods. This PR reduces the matrix methods to wrappers which simply call into the standalone functions to execute the calculations. This means that in the future if we wish to restrict all internal calculations to immutable types then all the wrappers would have to do would be to sympify `self` or otherwise convert `self` to immutable (if not already) before calling these underlying functions. Sympification can not be done currently due to a reason I will detail below.

* I have been asked to create several top-level standalone functions for matrix operations along the lines of `simplify(M)` as opposed to `M.simplify()`. These functions like the new matrix methods are simply wrappers which call the underlying concrete implementation, and like the wrappers they can eventually be changed to sympify their arguments to immutable types for caching.

* For caching as said currently the mutable and immutable matrix classes share a single entry point for their operations making it impossible to use `cacheit` to cache critical functions to speed up general operation (due to potential mutable returns). If the sympification problem is solved eventually and matrices can be cast to immutable by the wrappers before calling the underlying implementations then caching can be easily enabled by adding `cacheit` decorators to the relevant underlying private functions.

* For readability and maintainability as currently the monolithic `matrices/matrices.py` contains the implementations of almost all the complicated matrix functions. The file is roughly 5800 lines long and a pain-in-the-ass to understand / manage / merge. By converting the matrix methods in this file to wrappers and relocating the implementations into another source file all these things become much easier. Essentially it is converting those methods in `matrices.py` into a rough `.h` file type declaration and the actual definition lives in another file.

There are some other smaller independent changes made in this PR:

* This is the biggest and possibly most contentious one: The top level `det` function which used to reside in `matrices/expressions/determinant.py` has been changed to import from `matrices/determinant.py` so it can live alongside its other determinant-related functions. The functionality is the same just done in a slightly different way. This new top-level `det` function now detects whether a determinant for a concrete matrix is requested or if the argument is an abstract expression matrix object, in which case it then creates a `Determinant` object. This does not change anything for the user, abstract expressions using `MatrixSymbol` or `BlockMatrix` will still get represented as a `Determinant(M)` class whereas concrete matrices will return the determinant as before, except that they now accept the same control arguments as the matrix method `det` function does to be consistent with the matrix method. Imports like `from sympy.matrices.expressions import det` and `from sympy.matrices.expressions.determinant import det` still work since I left a `det` in expressions which is an alias for `sympy.matrices.determinant.det` for backwards compatibility. In my mind this also makes sense since `expression` is a subpackage of `matrices` and the calls should be flowing down instead of up. To reiterate, the resulting top-level `det` function works in exactly the same way except that it now allows parameters for controlling the determinant for concrete matrices consistent with their `det` method.

* `_find_reasonable_pivot` and `_find_reasonable_pivot_naive` have been turned into matrix methods rather than standalone functions due to the fact that sparse matrices can probably implement them more efficiently than the current dense matrix implementation and thus the implementation should be decided by the matrix class.

* `test_matrices.py` is an a large and annoying file, I pulled out the determinant tests into a new separate `test_determinant.py` file which also tests the new top-level standalone functions.

As for the matrix sympification problem: Currently code in other parts of the codebase (`polys/solvers.py`) creates and operates on matrices containing objects derived from `CantSympify` in order to solve linear systems using `RawMatrix.rref`. These objects raise an exception if a `sympify` is attempted on a matrix containing them, thus blocking sympification for intermediate calculations. The idea is to eventually replace the use of `RawMatrix` with a standalone `rref_list` function (this would be a `MatrixReductions` reorg like this) that does not directly use the Matrix classes but rather a list of lists. I actually have this done already but it would be too much for one PR so would rather come after in a `MatrixReductions` PR.

Conclusion: As stated the main purpose of this PR is to decouple the determinant related group of matrix functions from the current matrix class implementation to facilitate future matrix changes. The new top-level functions are a secondary change and are optional, as is the conversion of the `_find_reasonable_pivot` functions to methods. If this PR and methodology is accepted then this change would be carried out for the majority of important matrix functions residing in `MatrixReductions`, `MatrixSubspaces`, `MatrixEigen`, maybe `MatrixCalculus` and several classes of functions currently residing in `MatrixBase`. No functionality or signatures have been harmed in the production of this PR, that is to say this is a reorg and no operational changes to the actual functions or their call-signature have been made (other than readability).

#### Other comments

#### Release Notes
<!-- BEGIN RELEASE NOTES -->
* matrices
  * Pulled many `MatrixDeterminant` function implementations out of `matrices/matrices.py` into a separate `matrices/determinant.py` source file for easier maintenance and future modification (caching, move to immutable matrices and separate mutable).
<!-- END RELEASE NOTES -->
Pulled MatrixDeterminant function implementations out of matrix API
issue
created
Pristine-Cat
username_0
Sorry, committed to the wrong branch...
comment
created
Pristine-Cat
username_0
BTW, if this is accepted and I can move on to the next part `MatrixReductions` then that will solve the matrix sympify problem since the separated `_row_reduce_list` will be able to be used in `sympy/polys/solvers.py` which will allow intermediate calculations with immutable matrices and finally `cacheit`.
comment
created
oscarbenjamin
username_1
@username_3 and I have both said that we would like others to review the general idea of this PR. If this is accepted then it starts us down a road of moving large amounts of code out of the matrix classes and into standalone functions. I think that is a good way to go but I want to be sure that others are aware of this.

The basic idea here is that by going from `Matrix(...).jordan_form()` to `jordan_form(Matrix(...))` we move the nontrivial code out of the matrix classes. That makes it possible for a matrix class with a more limited interface to be usable still. In turn that makes it easier to define new matrix classes. Introducing new matrix classes makes it possible to fix many problems of the matrices API without breaking backwards compatibility.

If no one is interested to review this then @username_3, @username_0 and I can resume working on this. I just want everyone to be aware that this PR represents the start of a significant change in the codebase that involves moving a lot of code around and introducing a lot of new API.
comment
created
jksuom
username_2
+1
comment
created
Pristine-Cat
username_0
We have discussed this fairly thoroughly so I think you know where I stand, though there is one thing which would make this less painful to those who worry much (rightly so) about the public API, *cough* ... @username_3 ... :)

I am 50/50 on this but is there a real necessity to introduce the new top-level public functions at this point? If the eventual goal is to do a matrix 2.0 then do we want to tie the top-level function signatures to the current matrix implementation (think simplification args)? As you can see the top-level functions are nothing more than wrappers anyway, they can easily be added at any time and most of the work related to that is tests and docs. If we defer this step then we also don't have to tackle global namespace collisions like `det` yet, but rather I can go full speed with reorganizing the internals and the top-level decision can be made once we know the layout of the eventual new classes.
comment
created
sylee957
username_3
Yes, it's the only doubt that I would have, if the methods only work with the same explicit matrix classes. But I may agree on merging if you want to continue the work to see the full picture, and get less bothered with the decision problems.

But I'd expect to see the API to be symmetrical before the next release, and if that means a lot of work to be done but if you want the work to be done part-by-part, it may better be merged to a branch of sympy repo, than the master.
comment
created
Pristine-Cat
username_0
Hmm, repo, sounds like a place where ideas go to die... From what I understand you suggest I would do the work part by part there and once it is done it would be merged into master? That could be one ugly merge. Though if you are looking for an idea of how this would look eventually then that already exists in part - #15833 - though using wrappers instead of direct function assignment like in that PR for the matrix methods.

How about this instead, if @username_1 agrees to defer the API changes (top-level public functions) then I do the work part by part in chunks that make sense, but in master, only affecting internal operations, no user-facing functionality or interfaces would change. This would also allow me to keep up with master with each new chunk and make the merges much easier and I would not have to worry about getting too far ahead.  The chunks would be pulling the internal functions out of each individual class one at a time `MatrixDeterminant`, `MatrixReductions`, etc...

So essentially the pure work of pulling the internal implementations into separate files, nothing else.
comment
created
oscarbenjamin
username_1
I think that sounds good. Adding public functions (or simply renaming internal functions) is easy enough to do later.
comment
created
Pristine-Cat
username_0
Have a look @sylee, reverted everything. Even removed the public matrix methods that we had added `det_bareiss` and the other two and reverted them to their current form on master as `_eval_det_bareiss`, etc... All that is left is the relocation of the implementations to a separate file as private functions.
comment
created
Pristine-Cat
username_0
Next step #18420
comment