Opened 14 months ago

Closed 3 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:

Status badges

Description (last modified by gh-mwageringel)

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 14 months ago by slelievre

  • Cc frederichan slelievre added

comment:2 Changed 14 months ago by mkoeppe

  • Cc egourgoulhon added

comment:3 Changed 13 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:4 Changed 7 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:5 Changed 3 months ago by gh-mwageringel

  • Authors set to Markus Wageringel
  • Branch set to u/gh-mwageringel/30133
  • Cc charpent added
  • Commit set to a7e211eecfe241ddea4512ae6acc289f42efed6e
  • Description modified (diff)
  • Status changed from new to needs_review

Here is a basic fix which follows the implementation of the Maxima interface by changing the names of variables in the interface.


New commits:

6df116030133: rename internal variables in Giac interface to avoid clashes with constants
a7e211e30133: fix conversion of polynomials to Giac

comment:6 follow-up: Changed 3 months ago by tscrim

  • 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 3 months ago by git

  • Commit changed from a7e211eecfe241ddea4512ae6acc289f42efed6e to aaf26b907ca7f148f862e1b52d434e9e7b573430

Branch pushed to git repo; I updated commit sha1. New commits:

aaf26b930133: fix pyflakes warning

comment:8 in reply to: ↑ 6 Changed 3 months ago by gh-mwageringel

Thanks for the review.

Replying to tscrim:

LGTM overall. One little thing is adding # -*- coding: utf-8 -*- to the first line of src/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 3 months ago by tscrim

  • 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 3 months ago by vbraun

  • Branch changed from u/gh-mwageringel/30133 to aaf26b907ca7f148f862e1b52d434e9e7b573430
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.