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

Status badges

Description (last modified by mantepse)

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 2 years ago by mantepse

  • Branch set to u/mantepse/conversion_of_symbolic_ring_to_fricas_expression_integer

comment:2 Changed 2 years ago by mantepse

  • Authors set to Martin Rubey
  • 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

New commits:

4c672a0make sagemath SR convert to FriCAS EXPR INT

comment:3 follow-up: Changed 2 years ago by tmonteil

Possible collision with #28641 which refers to the same ask.sagemath.org question.

comment:4 in reply to: ↑ 3 Changed 2 years ago by mantepse

Replying to tmonteil:

Possible collision with #28641 which refers to the same ask.sagemath.org question.

Yes, but the solutions are orthogonal to each other. In other words: after applying this branch, #28641 solves a different problem.

comment:5 Changed 2 years ago by chapoton

some failing doctests in src/sage/functions/generalized.py and src/sage/functions/exp_integral.py

comment:6 Changed 2 years ago by mantepse

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 mantepse

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:

  1. rewrite I as sqrt(-1),
  2. always use Expression Complex Integer,
  3. detect whether I appears and distinguish accordingly,
  4. somehow let FriCAS decide

I tend towards option 1., but I am not sure at all.

comment:8 Changed 2 years ago by mantepse

  • Status changed from needs_review to needs_work

help appreciated

comment:9 Changed 2 years ago by git

  • Commit changed from 4c672a0d25b00b0f33c873a71164d5b6e6ac5916 to 8948d3883678895a95fcc67f72801375e3f23832

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

8948d38try to fix conversion of complex expressions to fricas

comment:10 Changed 2 years ago by mantepse

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:11 Changed 2 years ago by mantepse

  • 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 git

  • Commit changed from 8948d3883678895a95fcc67f72801375e3f23832 to d1b449088d9423f1f8faadd0d6e09a1ed873ca30

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

8601700fix doctests
4205759Merge 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
d1b4490Merge branch 'develop' into t/28647/conversion_of_symbolic_ring_to_fricas_expression_integer

comment:13 Changed 2 years ago by mantepse

  • Keywords FriCAS added

comment:14 Changed 2 years ago by git

  • Commit changed from d1b449088d9423f1f8faadd0d6e09a1ed873ca30 to b95f2a46b2a7eecdc3c42d042fae1031b3ade548

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

b95f2a4make an exception for symbols

comment:15 Changed 2 years ago by mantepse

  • Status changed from needs_work to needs_review

comment:16 Changed 2 years ago by git

  • Commit changed from b95f2a46b2a7eecdc3c42d042fae1031b3ade548 to 09bee2611190ef2d5071427ac69b6a8927e4fece

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

09bee26fix doctests

comment:17 Changed 22 months ago by embray

  • Milestone changed from sage-9.0 to sage-9.1

Ticket retargeted after milestone closed

comment:18 Changed 18 months ago by mkoeppe

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

  • Milestone changed from sage-9.2 to sage-9.3

comment:20 Changed 12 months ago by chapoton

  • Status changed from needs_review to needs_work

red branch => needs work

comment:21 Changed 12 months ago by git

  • Commit changed from 09bee2611190ef2d5071427ac69b6a8927e4fece to da22da29c47948d320f6cc14a618954be8923eba

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

da22da2Merge branch 'develop' into t/28647/conversion_of_symbolic_ring_to_fricas_expression_integer

comment:22 Changed 12 months ago by mantepse

  • Status changed from needs_work to needs_review

comment:23 Changed 12 months ago by mantepse

  • Status changed from needs_review to needs_work

Unfortunately, #18036 breaks this.

comment:24 Changed 12 months ago by mantepse

(and I have no idea currently how to fix this)

comment:25 Changed 8 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:26 Changed 5 months ago by git

  • Commit changed from da22da29c47948d320f6cc14a618954be8923eba to 1624c4524e64159b8b5a9d3ce788bf587fc41f3d

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

f9c4dfdMerge branch 'develop' into t/28647/conversion_of_symbolic_ring_to_fricas_expression_integer
1624c45adapt to new parent of I

comment:27 Changed 5 months ago by mantepse

  • Status changed from needs_work to needs_review

comment:28 Changed 5 months ago by tscrim

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 git

  • Commit changed from 1624c4524e64159b8b5a9d3ce788bf587fc41f3d to 0ead4585cdb61bf9e361a833bd5472f5dd18a781

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

0ead458simplify test for complex i

comment:30 Changed 5 months ago by mantepse

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 git

  • Commit changed from 0ead4585cdb61bf9e361a833bd5472f5dd18a781 to c92aec8046b9060eb6c30d99520430ca2f96745e

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

c92aec8remove superfluous import of I

comment:32 Changed 5 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thank you. LGTM.

comment:33 Changed 5 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.