Opened 2 years ago
Closed 14 months ago
#30133 closed defect (fixed)
Giac mixes variables and constants of the same name such as I, e, π
Reported by: | gh-mwageringel | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.4 |
Component: | symbolics | Keywords: | giac |
Cc: | frederichan, slelievre, egourgoulhon, charpent | Merged in: | |
Authors: | Markus Wageringel | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | aaf26b9 (Commits, GitHub, GitLab) | Commit: | aaf26b907ca7f148f862e1b52d434e9e7b573430 |
Dependencies: | Stopgaps: |
Description (last modified by )
The Giac interface needs to be more careful when handling symbolic variables that are also mathematical constants:
sage: var('I')._giac_()._sage_()^2 # should be I^2 -1 sage: var('e')._giac_()._sage_().n() # should not evaluate 2.71828182845905 sage: var('π')._giac_()._sage_().n() # should not evaluate 3.14159265358979
sage: e, x = var('e,x') sage: integrate(e^x, x, algorithm='maxima') # correct e^x/log(e) sage: integrate(e^x, x, algorithm='giac') # wrong e^x
sage: y = var('π') sage: integrate(cos(y), y, algorithm='maxima') # correct (after ticket 30112) sin(π) sage: integrate(cos(y), y, algorithm='giac') # wrong -pi
This also came up in this ask-sage question.
To avoid this conflict, this ticket changes the names of the internal variables in the Giac interface, which mirrors the behavior of the Maxima interface.
sage: giac(SR.var('e')) sageVARe sage: _.sage() e
Change History (10)
comment:1 Changed 2 years ago by
- Cc frederichan slelievre added
comment:2 Changed 2 years ago by
- Cc egourgoulhon added
comment:3 Changed 2 years ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:4 Changed 18 months ago by
- Milestone changed from sage-9.3 to sage-9.4
comment:5 Changed 14 months ago by
- Branch set to u/gh-mwageringel/30133
- Cc charpent added
- Commit set to a7e211eecfe241ddea4512ae6acc289f42efed6e
- Description modified (diff)
- Status changed from new to needs_review
comment:6 follow-up: ↓ 8 Changed 14 months ago by
- Reviewers set to Travis Scrimshaw
LGTM overall. One little thing is adding # -*- coding: utf-8 -*-
to the first line of src/sage/symbolic/integration/external.py
. A while-we-are-at-it thing that you can ignore is fixing the pyflakes warnings in src/sage/symbolic/expression_conversions.py
. Once done, you can set a positive review on my behalf.
comment:7 Changed 14 months ago by
- Commit changed from a7e211eecfe241ddea4512ae6acc289f42efed6e to aaf26b907ca7f148f862e1b52d434e9e7b573430
Branch pushed to git repo; I updated commit sha1. New commits:
aaf26b9 | 30133: fix pyflakes warning
|
comment:8 in reply to: ↑ 6 Changed 14 months ago by
Thanks for the review.
Replying to tscrim:
LGTM overall. One little thing is adding
# -*- coding: utf-8 -*-
to the first line ofsrc/sage/symbolic/integration/external.py
.
In Python 3, utf-8 encoding is the default, so this line is not needed.
I have fixed the pyflakes warning, though.
comment:9 Changed 14 months ago by
- Status changed from needs_review to positive_review
I am not fully convinced and would rather be safe, but it's not a hill I am willing to die on as I don't know of something that definitely breaks right now that we support.
comment:10 Changed 14 months ago by
- Branch changed from u/gh-mwageringel/30133 to aaf26b907ca7f148f862e1b52d434e9e7b573430
- Resolution set to fixed
- Status changed from positive_review to closed
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.