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:  ghmwageringel  Owned by:  

Priority:  major  Milestone:  sage9.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 asksage 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 sage9.2 to sage9.3
comment:4 Changed 18 months ago by
 Milestone changed from sage9.3 to sage9.4
comment:5 Changed 14 months ago by
 Branch set to u/ghmwageringel/30133
 Cc charpent added
 Commit set to a7e211eecfe241ddea4512ae6acc289f42efed6e
 Description modified (diff)
 Status changed from new to needs_review
comment:6 followup: ↓ 8 Changed 14 months ago by
 Reviewers set to Travis Scrimshaw
LGTM overall. One little thing is adding # * coding: utf8 *
to the first line of src/sage/symbolic/integration/external.py
. A whileweareatit 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: utf8 *
to the first line ofsrc/sage/symbolic/integration/external.py
.
In Python 3, utf8 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/ghmwageringel/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.