Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

conversion of Symbolic Ring to FriCAS Expression Integer #28647

Closed
mantepse opened this issue Oct 24, 2019 · 41 comments
Closed

conversion of Symbolic Ring to FriCAS Expression Integer #28647

mantepse opened this issue Oct 24, 2019 · 41 comments

Comments

@mantepse
Copy link
Collaborator

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")

CC: @fchapoton

Component: interfaces: optional

Keywords: FriCAS

Author: Martin Rubey

Branch/Commit: c92aec8

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/28647

@mantepse mantepse added this to the sage-9.0 milestone Oct 24, 2019
@mantepse
Copy link
Collaborator Author

@mantepse

This comment has been minimized.

@mantepse
Copy link
Collaborator Author

New commits:

4c672a0make sagemath SR convert to FriCAS EXPR INT

@mantepse
Copy link
Collaborator Author

Commit: 4c672a0

@mantepse
Copy link
Collaborator Author

Author: Martin Rubey

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Oct 24, 2019

comment:3

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

@mantepse
Copy link
Collaborator Author

comment:4

Replying to @sagetrac-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.

@fchapoton
Copy link
Contributor

comment:5

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

@mantepse
Copy link
Collaborator Author

comment:6

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.

@mantepse
Copy link
Collaborator Author

comment:7

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.

@mantepse
Copy link
Collaborator Author

comment:8

help appreciated

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 16, 2019

Changed commit from 4c672a0 to 8948d38

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 16, 2019

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

8948d38try to fix conversion of complex expressions to fricas

@mantepse

This comment has been minimized.

@mantepse
Copy link
Collaborator Author

comment:11

I think I have to make an exception for the conversion of symbols, because otherwise fricas.integrate(ex, x) won't work anymore.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 20, 2019

Changed commit from 8948d38 to d1b4490

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 20, 2019

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

@mantepse
Copy link
Collaborator Author

Changed keywords from none to FriCAS

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 20, 2019

Changed commit from d1b4490 to b95f2a4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2020

Changed commit from 09bee26 to da22da2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2020

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

@mantepse
Copy link
Collaborator Author

mantepse commented Nov 2, 2020

comment:23

Unfortunately, #18036 breaks this.

@mantepse
Copy link
Collaborator Author

mantepse commented Nov 2, 2020

comment:24

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

@mkoeppe
Copy link
Member

mkoeppe commented Feb 13, 2021

comment:25

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

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Feb 13, 2021
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 31, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 31, 2021

Changed commit from da22da2 to 1624c45

@tscrim
Copy link
Collaborator

tscrim commented Jun 1, 2021

comment:28

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2021

Changed commit from 1624c45 to 0ead458

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2021

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

0ead458simplify test for complex i

@mantepse
Copy link
Collaborator Author

mantepse commented Jun 1, 2021

comment:30

Yes, you are right! I copied these lines from the generic class InterfaceInit, so I applied your suggestion there, too.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2021

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

c92aec8remove superfluous import of I

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2021

Changed commit from 0ead458 to c92aec8

@tscrim
Copy link
Collaborator

tscrim commented Jun 1, 2021

comment:32

Thank you. LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Jun 1, 2021

Reviewer: Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Jun 7, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants