Opened 3 years ago
Closed 12 months ago
#28647 closed defect (fixed)
conversion of Symbolic Ring to FriCAS Expression Integer
Reported by: | mantepse | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.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/why-this-integral-fail-using-fricas-algorithm/
for the original bug report:
integrate(sqrt(2)*x^2 + 2*x,x, algorithm="fricas")
Change History (33)
comment:1 Changed 3 years ago by
- Branch set to u/mantepse/conversion_of_symbolic_ring_to_fricas_expression_integer
comment:2 Changed 3 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 follow-up: ↓ 4 Changed 3 years ago by
Possible collision with #28641 which refers to the same ask.sagemath.org question.
comment:4 in reply to: ↑ 3 Changed 3 years ago by
comment:5 Changed 3 years ago by
some failing doctests in src/sage/functions/generalized.py and src/sage/functions/exp_integral.py
comment:6 Changed 3 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 3 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 3 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 3 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:11 Changed 3 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 2 years ago by
- Milestone changed from sage-9.0 to sage-9.1
Ticket retargeted after milestone closed
comment:18 Changed 2 years ago by
- Milestone changed from sage-9.1 to sage-9.2
Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.
comment:19 Changed 21 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:20 Changed 19 months ago by
- Status changed from needs_review to needs_work
red branch => needs work
comment:21 Changed 19 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 19 months ago by
- Status changed from needs_work to needs_review
comment:23 Changed 19 months ago by
- Status changed from needs_review to needs_work
Unfortunately, #18036 breaks this.
comment:24 Changed 19 months ago by
(and I have no idea currently how to fix this)
comment:25 Changed 15 months ago by
- 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:26 Changed 12 months ago by
- Commit changed from da22da29c47948d320f6cc14a618954be8923eba to 1624c4524e64159b8b5a9d3ce788bf587fc41f3d
comment:27 Changed 12 months ago by
- Status changed from needs_work to needs_review
comment:28 Changed 12 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 12 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 12 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 12 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 12 months ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Thank you. LGTM.
comment:33 Changed 12 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