-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Fixed LaTeX- and pretty-printing of Determinant #22773
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
Conversation
✅ Hi, I am the SymPy bot (v163). 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:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.11. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
There are things that must be fixed here... |
Now it should be working correctly. I noted that this happens:
so an extra parenthesis for Inverse. This is fixed here so that it is printed as
(similar for LaTeX) and should be fixed similarly for Adjoint and Transpose (at least), but since #22772 changes them, it will have to wait. |
92deba4
to
bdbb2ff
Compare
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) Full benchmark results can be found as artifacts in GitHub Actions |
Is this ready if #22772 is in? |
I think/hope so. As far as I know, the combination should not break anything. (If it does, I'll update the tests ASAP.) |
sympy/printing/latex.py
Outdated
else: | ||
mat_delim_backup = self._settings['mat_delim'] | ||
self._settings['mat_delim'] = '' | ||
tex = r"\left|{%s}\right|" % self._print(expr.args[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mentioned this on the issue, but why not just set the delimiter to |
and recursively print expr.args[0]
? The result would be the same but would be less convoluted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main reason is that it was like that in the code.
The corresponding code would be:
self._settings['mat_delim'] = '|'
tex = self._print(expr.args[0])
and changing
self._delim_dict = {'(': ')', '[': ']'}
to
self._delim_dict = {'(': ')', '[': ']', '|': '|'}
so to me it seemed about the same.
But I can definitely change it. One should probably check how block matrices print though... Things like
X = MatrixSymbol('X', 2, 2)
m = Matrix(((1, 2), (3, 4)))
BlockMatrix(((OneMatrix(2, 2), X), (m, ZeroMatrix(2, 2))))
Will probably not print in a sensible way independent of solution...
(Right now:
⎡ 𝟙 X⎤
⎢ ⎥
⎢⎡1 2⎤ ⎥
⎢⎢ ⎥ 𝟘⎥
⎣⎣3 4⎦ ⎦
)
sympy/printing/latex.py
Outdated
tex = r"\left|{%s}\right|" % self._print(expr.args[0]) | ||
self._settings['mat_delim'] = mat_delim_backup | ||
if exp is not None: | ||
return r"%s^{%s}" % (tex, exp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Pow printer only uses the exp argument if the argument is a Function. Maybe this could be generalized to allow other classes, although in this case, do we really want powers of determinants to print like |A|^2
? A test you added below seems to indicate not. Personally I'd say we should not print determinant powers like that because it makes it unclear if an expression is actually Determinant(MatPow)
or Pow(Determinant)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plan was that it should be printed as |A|^2
, yes. And the other way around as |A^2|
(or |(A)^2|
if A is a Matrix
rather than a MatrixSymbol
).
Probably adding the exp-argument was a bit too much copy and paste...
1431de1
to
c87299c
Compare
This is now refactored to not fiddle with the settings and handle block matrices in a much better way now. |
c87299c
to
ab126b2
Compare
References to other Issues or PRs
Closes #22763
Brief description of what is fixed or changed
Corrected LaTeX-printing of Determinant.
Other comments
I seem to recall that there was some discussion about this elsewhere, but cannot really recall where...
Release Notes