-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
make symbolic matrices use pynac symbolics #6115
Comments
comment:1
Mike Hansen mentions that all of the docstrings should be moved up to the module docstring. Most of this patch would probably be just deleting functions in matrix_symbolic_dense.pyx. |
comment:2
Almost positive review. Here is a problem:
|
comment:3
I should mention that all but about 3 lines of the above patch are Mike Hansen's, so I'm not reviewing my own patch! I did add the simplify_rational() calls to the exponent doctests and the deprecation warning to is_simplified; those should be reviewed by someone other than me. |
comment:4
Playing around with things gives more interesting errors:
|
apply on top of previous patches |
comment:5
Attachment: trac-6115-symbolic-eigenvalues.patch.gz the trac-6115-symbolic-eigenvalues.patch patch needs to be reviewed as well. |
comment:6
the complaints that I raise above happened with the old implementation of symbolic matrices, so they are beyond the scope of this ticket. I positively review mhansen's part (the first patch, which I posted). My part (the second patch) needs to be reviewed, though. |
comment:7
(well, and the three or so lines that I changed from mhansen's patch; see my above comment) |
comment:8
Jason's patch looks good to me. I just put up a new version of #6115 which fixes a doctest failure in sage/matrix/tests.py. |
comment:9
Yep, and Mike's change looks good to me and makes tests.py pass doctests, so positive review. |
Attachment: trac_6115.patch.gz |
comment:10
Merged both patches in 4.0.rc1. |
Currently, it looks like the symbolic matrix code in
matrix/matrix_symbolic_dense.pyx
goes to maxima for everything. This makes things slow and, at least in 3.4.2, it was easy to crash Maxima by calculating a 6x6 determinant, for example.Here is an example of how the current general algorithms in Sage can speed things up (even before making the matrix be stored in Sage instead of Maxima).
I went into matrix/matrix_symbolic_dense.pyx and just commented out the determinant routine. This way, it uses the generic determinant routine for matrices. Note that we still have to get values from maxima for this, but the multiplication and things are done in pynac.
Generic determinant algorithm:
So, the generic Sage code with pynac took about 3% of the time it took to call maxima and ask it for the determinant.
Component: linear algebra
Issue created by migration from https://trac.sagemath.org/ticket/6115
The text was updated successfully, but these errors were encountered: