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

Convert symbolic relations to/from SymPy #23990

Closed
rwst opened this issue Oct 8, 2017 · 28 comments
Closed

Convert symbolic relations to/from SymPy #23990

rwst opened this issue Oct 8, 2017 · 28 comments

Comments

@rwst
Copy link
Contributor

rwst commented Oct 8, 2017

sage: (x>0)._sympy_()
...
NotImplementedError: relation

Reversely in SymPy:

In [9]: x >= 0
Out[9]: x ≥ 0

In [10]: _._sage_()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-10-7149a2430cab> in <module>()
----> 1 _._sage_()

AttributeError: 'GreaterThan' object has no attribute '_sage_'

In [11]: Eq(x, 3)
Out[11]: x = 3

In [12]: _._sage_()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-12-7149a2430cab> in <module>()
----> 1 _._sage_()

AttributeError: 'Equality' object has no attribute '_sage_'

This functionality is needed both by #22322 and #23923. Needed is a fix to SympyConverter in expression-conversions.pyx

The reverse patch in SymPy is added here, see the PR sympy/sympy#13420

CC: @mforets

Component: interfaces

Author: Ralf Stephan

Branch/Commit: 479e206

Reviewer: Emmanuel Charpentier

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

@rwst rwst added this to the sage-8.1 milestone Oct 8, 2017
@rwst
Copy link
Contributor Author

rwst commented Oct 8, 2017

@rwst
Copy link
Contributor Author

rwst commented Oct 8, 2017

New commits:

946295423990: Convert symbolic relations to SymPy

@rwst
Copy link
Contributor Author

rwst commented Oct 8, 2017

Commit: 9462954

@rwst

This comment has been minimized.

@rwst rwst changed the title Convert symbolic relations to/from !SymPy Convert symbolic relations to/from SymPy Oct 8, 2017
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 8, 2017

Changed commit from 9462954 to 07f4744

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 8, 2017

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

07f474423990: handle unequality

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 8, 2017

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

0ed659f23990: do not evaluate

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 8, 2017

Changed commit from 07f4744 to 0ed659f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 9, 2017

Changed commit from 0ed659f to 5f023ea

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 9, 2017

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

5f023ea23990: convert relations from SymPy to Sage, with test

@rwst

This comment has been minimized.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Oct 9, 2017

comment:7

8.1.beta7 + #23990 passes ptestlong on Debian testing running on core i5 + 8 GB RAM with no error whatsoever.

==>positive_review

PS : You need to put your name in "Authors"

PPS : Thank you very much ! Solving inequations is a domain where Sage and Maxima are currenty weak. This is a nice step in the right direction.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Oct 9, 2017

Reviewer: Emmanuel Charpentier

@rwst
Copy link
Contributor Author

rwst commented Oct 10, 2017

comment:8

Thanks!

@rwst
Copy link
Contributor Author

rwst commented Oct 10, 2017

Author: Ralf Stephan

@rwst
Copy link
Contributor Author

rwst commented Oct 10, 2017

comment:9

P.S. Don't forget to reinstall SymPy before compiling Sage.

@rwst
Copy link
Contributor Author

rwst commented Oct 10, 2017

comment:10

Sorry, I forgot to increase the SymPy patchlevel, and the patch does no apply anyway.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2017

Changed commit from 5f023ea to 0f596bb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2017

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

0f596bb23990: fix patch

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2017

Changed commit from 0f596bb to 479e206

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2017

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

479e20623990: sympy patchlevel bump

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Oct 10, 2017

comment:15

Replying to @rwst:

Sorry, I forgot to increase the SymPy patchlevel, and the patch does no apply anyway.

Ah ! I thought that the reverse conversion (which I saw on the sympy site) was reserved for another Sage patch...

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Oct 10, 2017

comment:16
sage: var("a,b")
(a, b)
sage: sympy.sympify(a==b)._sage_()
a == b
sage: sympy.sympify(a==b).sage()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-14-25de6a4c8f0a> in <module>()
----> 1 sympy.sympify(a==b).sage()

AttributeError: 'Equality' object has no attribute 'sage'

Is that the intended behaviour ? Or to you plan to overhaul a .sage() method for various Sympy objects ?

It is not obvious to me why (e. g.) Mathematic objects can be converted back to Sage objects via the (exposed) .sage() method, whereas one has to use the (unexposed) ._sage_() methods for Sympy objects...

@rwst
Copy link
Contributor Author

rwst commented Oct 10, 2017

comment:17

_sage_() is just the SymPy convention, every convertible object has it. They have a lot of other underscore member functions, try grep 'def _' sympy/*/*/*. Are Mma objects not Sage objects? Should we enforce coding conventions between projects?

Please don't forget to set back positive.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Oct 10, 2017

comment:18

Replying to @rwst:

_sage_() is just the SymPy convention, every convertible object has it. They have a lot of other underscore member functions, try grep 'def _' sympy/*/*/*.

Indeed. No quarell here.

Are Mma objects not Sage objects?

They are by definition Sage objects, but have no meaning for interacting with Sage "native" objects ; a conversion is needed for that.

Should we enforce coding conventions between projects?

I think that's almost what I was thinking about (syntax consistency rather than coding conventions).

Note that the non-Sympy interfaces are not that consistent : compare, for example, interfaces to mathematica, giac, R, maple and fricas... Unifying them would represent a lot of work second-intention consistency is hard...

Another reason was that, unless I'm mistaken, the docs I read when I was starting to learn Sage (about 2-3 years ago) recommended to "end users" to use only "exposed" methods, leaving the unexposed ones to internal Sage use. I certainly had this recommendation in mind when I asked my question.

Has this recommendation changed ?

Please don't forget to set back positive.

Of course ;-).

And, BTW, this was only a question, not a criticism...

@rwst
Copy link
Contributor Author

rwst commented Oct 10, 2017

comment:19

The problem is that you seem to want a way to know how to convert a Python object from a different namespace by asking for its methods. But Sage has no control over them because Python does not allow adding methods on-the-fly. It may be an idea to just provide a Sage global function to_sage(...) that knows these members and calls them without you having to know. Please open a ticket or a discussion and cite me as much as you want. This ticket has fulfilled its purpose.

@rwst
Copy link
Contributor Author

rwst commented Oct 10, 2017

comment:20

Replying to @rwst:

...because Python does not allow adding methods on-the-fly...

I'm a Python noob. I didn't know about MethodType.

@vbraun
Copy link
Member

vbraun commented Oct 20, 2017

Changed branch from u/rws/convert_symbolic_relations_to_from__sympy to 479e206

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

2 participants