Fix printing bugs around the gsym argument to Ga#340
Merged
eric-wieser merged 3 commits intopygae:masterfrom May 18, 2020
Merged
Fix printing bugs around the gsym argument to Ga#340eric-wieser merged 3 commits intopygae:masterfrom
eric-wieser merged 3 commits intopygae:masterfrom
Conversation
|
Check out this pull request on Review Jupyter notebook visual diffs & provide feedback on notebooks. Powered by ReviewNB |
This comment has been minimized.
This comment has been minimized.
eric-wieser
force-pushed
the
fix-gsym-bugs
branch
3 times, most recently
from
2bc0f6c to
16c70bc
Compare
Codecov Report
@@ Coverage Diff @@
## master #340 +/- ##
=========================================
Coverage ? 75.88%
=========================================
Files ? 16
Lines ? 4500
Branches ? 0
=========================================
Hits ? 3415
Misses ? 1085
Partials ? 0
Continue to review full report at Codecov.
|
eric-wieser
commented
May 15, 2020
5 tasks
utensil
reviewed
May 16, 2020
|
|
||
| def _print_Determinant(self, expr): | ||
| # sympy `uses |X|` by default, we want `det (X)` | ||
| return r"\det\left ( {}\right )".format(self._print(expr.args[0])) |
Member
There was a problem hiding this comment.
These two styles are just two styles, does SymPy has an option for it？
Member
Author
There was a problem hiding this comment.
No, it does not. We could make a patch upstream to support an option, but for now it's easiest just to add it here.
This ran into three issues with sympy: * Determinant objects are not considered commutative, which breaks Mv. This is a bug. We work around it by making _our_ determinant subclass explicitly commutative * There is no MatrixFunction builtin. We define our own for now. * The default printing of `Determinant` isn't what we want. We can override it in our printer. This fixes: * the value of gsym affecting whether the printing is italic * the state of the printer at construction affecting the lifetime printing behavior
Previously this was computed once in the constructor, and once again, possibly with a completely different value, when constructing the reciprocal basis blades. Making this a cached_property ensures we compute it exactly once. This fixes the weird statefulness in the notebook included in this commit.
eric-wieser
force-pushed
the
fix-gsym-bugs
branch
from
16c70bc to
0a8a822
Compare
For some reason, the Metric was creating the symbol `|g|` while the Ga was creating the symbol `det(g)`. Merging these together makes the picture a lot clearer.
eric-wieser
force-pushed
the
fix-gsym-bugs
branch
from
0a8a822 to
8a91ea6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We should merge #339 first, putting up now to trial CI.See commit messages for an explanation of this change.