Opened 2 years ago
Closed 5 months ago
#28647 closed defect (fixed)
conversion of Symbolic Ring to FriCAS Expression Integer
Reported by:  mantepse  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  interfaces: optional  Keywords:  FriCAS 
Cc:  chapoton  Merged in:  
Authors:  Martin Rubey  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  c92aec8 (Commits, GitHub, GitLab)  Commit:  c92aec8046b9060eb6c30d99520430ca2f96745e 
Dependencies:  Stopgaps: 
Description (last modified by )
This ticket modifies the FriCAS interface so that symbolic ring elements are always converted to FriCAS Expression Integer
or Expression Complex Integer
, depending on whether the complex unit appears. Doing so, we fix the failure below.
see https://ask.sagemath.org/question/48431/whythisintegralfailusingfricasalgorithm/
for the original bug report:
integrate(sqrt(2)*x^2 + 2*x,x, algorithm="fricas")
Change History (33)
comment:1 Changed 2 years ago by
 Branch set to u/mantepse/conversion_of_symbolic_ring_to_fricas_expression_integer
comment:2 Changed 2 years ago by
 Cc chapoton added
 Commit set to 4c672a0d25b00b0f33c873a71164d5b6e6ac5916
 Component changed from PLEASE CHANGE to interfaces: optional
 Description modified (diff)
 Status changed from new to needs_review
 Type changed from PLEASE CHANGE to defect
comment:3 followup: ↓ 4 Changed 2 years ago by
Possible collision with #28641 which refers to the same ask.sagemath.org question.
comment:4 in reply to: ↑ 3 Changed 2 years ago by
comment:5 Changed 2 years ago by
some failing doctests in src/sage/functions/generalized.py and src/sage/functions/exp_integral.py
comment:6 Changed 2 years ago by
There are unfortunately more problems. In particular, fricas(I)
calls FriCASConverter.pyobject(I, I)
(as it should). But this now returns I
instead of %i
.
comment:7 Changed 2 years ago by
I'm afraid my approach won't work, because FriCAS distinguishes between Expression Integer
and Expression Complex Integer
.
Put differently, the complex unit I
is not an element of Expression Integer
, but would have to be expressed as sqrt(1)
. I see the following options:
 rewrite
I
assqrt(1)
,  always use
Expression Complex Integer
,  detect whether
I
appears and distinguish accordingly,  somehow let FriCAS decide
I tend towards option 1., but I am not sure at all.
comment:9 Changed 2 years ago by
 Commit changed from 4c672a0d25b00b0f33c873a71164d5b6e6ac5916 to 8948d3883678895a95fcc67f72801375e3f23832
Branch pushed to git repo; I updated commit sha1. New commits:
8948d38  try to fix conversion of complex expressions to fricas

comment:10 Changed 2 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:11 Changed 2 years ago by
 Status changed from needs_review to needs_work
I think I have to make an exception for the conversion of symbols, because otherwise fricas.integrate(ex, x)
won't work anymore.
comment:12 Changed 2 years ago by
 Commit changed from 8948d3883678895a95fcc67f72801375e3f23832 to d1b449088d9423f1f8faadd0d6e09a1ed873ca30
Branch pushed to git repo; I updated commit sha1. New commits:
8601700  fix doctests

4205759  Merge branch 'u/mantepse/conversion_of_symbolic_ring_to_fricas_expression_integer' of git://trac.sagemath.org/sage into t/28647/conversion_of_symbolic_ring_to_fricas_expression_integer

d1b4490  Merge branch 'develop' into t/28647/conversion_of_symbolic_ring_to_fricas_expression_integer

comment:13 Changed 2 years ago by
 Keywords FriCAS added
comment:14 Changed 2 years ago by
 Commit changed from d1b449088d9423f1f8faadd0d6e09a1ed873ca30 to b95f2a46b2a7eecdc3c42d042fae1031b3ade548
Branch pushed to git repo; I updated commit sha1. New commits:
b95f2a4  make an exception for symbols

comment:15 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:16 Changed 2 years ago by
 Commit changed from b95f2a46b2a7eecdc3c42d042fae1031b3ade548 to 09bee2611190ef2d5071427ac69b6a8927e4fece
Branch pushed to git repo; I updated commit sha1. New commits:
09bee26  fix doctests

comment:17 Changed 22 months ago by
 Milestone changed from sage9.0 to sage9.1
Ticket retargeted after milestone closed
comment:18 Changed 18 months ago by
 Milestone changed from sage9.1 to sage9.2
Moving tickets to milestone sage9.2 based on a review of last modification date, branch status, and severity.
comment:19 Changed 14 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:20 Changed 12 months ago by
 Status changed from needs_review to needs_work
red branch => needs work
comment:21 Changed 12 months ago by
 Commit changed from 09bee2611190ef2d5071427ac69b6a8927e4fece to da22da29c47948d320f6cc14a618954be8923eba
Branch pushed to git repo; I updated commit sha1. New commits:
da22da2  Merge branch 'develop' into t/28647/conversion_of_symbolic_ring_to_fricas_expression_integer

comment:22 Changed 12 months ago by
 Status changed from needs_work to needs_review
comment:23 Changed 12 months ago by
 Status changed from needs_review to needs_work
Unfortunately, #18036 breaks this.
comment:24 Changed 12 months ago by
(and I have no idea currently how to fix this)
comment:25 Changed 8 months ago by
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:26 Changed 5 months ago by
 Commit changed from da22da29c47948d320f6cc14a618954be8923eba to 1624c4524e64159b8b5a9d3ce788bf587fc41f3d
comment:27 Changed 5 months ago by
 Status changed from needs_work to needs_review
comment:28 Changed 5 months ago by
I believe you can simplify
 if (isinstance(obj, NumberFieldElement_quadratic) and  obj.parent() is GaussianField()): + if isinstance(obj, NumberFieldElement_gaussian):
I probably would also add a test that includes SR(I)
(or sqrt(1)
) to make sure that alternative version yields what you want. Other than that, LGTM.
comment:29 Changed 5 months ago by
 Commit changed from 1624c4524e64159b8b5a9d3ce788bf587fc41f3d to 0ead4585cdb61bf9e361a833bd5472f5dd18a781
Branch pushed to git repo; I updated commit sha1. New commits:
0ead458  simplify test for complex i

comment:30 Changed 5 months ago by
Yes, you are right! I copied these lines from the generic class InterfaceInit
, so I applied your suggestion there, too.
comment:31 Changed 5 months ago by
 Commit changed from 0ead4585cdb61bf9e361a833bd5472f5dd18a781 to c92aec8046b9060eb6c30d99520430ca2f96745e
Branch pushed to git repo; I updated commit sha1. New commits:
c92aec8  remove superfluous import of I

comment:32 Changed 5 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Thank you. LGTM.
comment:33 Changed 5 months ago by
 Branch changed from u/mantepse/conversion_of_symbolic_ring_to_fricas_expression_integer to c92aec8046b9060eb6c30d99520430ca2f96745e
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
make sagemath SR convert to FriCAS EXPR INT