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

_sympy_ methods for some parent classes #31931

Closed
mkoeppe opened this issue Jun 8, 2021 · 29 comments
Closed

_sympy_ methods for some parent classes #31931

mkoeppe opened this issue Jun 8, 2021 · 29 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 8, 2021

We add _sympy_ methods to various parent classes, returning sympy Integers, Reals, Complexes, ProductSet.

These methods override the generic _sympy_ method provided by #31938.

Part of #31926 Meta-ticket: Connect Sage sets to sympy sets

Depends on #31938
Depends on #31877

CC: @tscrim @kcrisman @certik @videlec

Component: interfaces

Author: Matthias Koeppe

Branch/Commit: 32cdd5c

Reviewer: Karl-Dieter Crisman

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

@mkoeppe mkoeppe added this to the sage-9.4 milestone Jun 8, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 9, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 9, 2021

Commit: 9fcf32e

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 9, 2021

New commits:

9fcf32eSets.CartesianProducts.ParentMethods, FreeModule_ambient, IntegerRing_class, InternalRealInterval, RealSet, NonNegativeIntegers, IntegerRing_class, PositiveIntegers, RationalField: Add `_sympy_` methods

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 9, 2021

Author: Matthias Koeppe

@kcrisman
Copy link
Member

kcrisman commented Jun 9, 2021

comment:3

Nice - if patchbot says yes this this seems to be good.

Question: Do these reimport into Sage as one might expect (does the diagram commute in either direction)? If so, some tests for that might be appropriate. Also, with the products, what happens if one of the parts doesn't have a Sympy version - presumably there is an error message, but is it a useful one?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 9, 2021

comment:4

Replying to @kcrisman:

Do these reimport into Sage as one might expect (does the diagram commute in either direction)?

No, the other direction of conversion is not implemented yet, this would be part of #31935. The closest we have is sage.interfaces.sympy.sympy_set_to_list, which converts from various sympy set types to lists of Sage SR relation expressions.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 9, 2021

comment:5

Replying to @kcrisman:

with the products, what happens if one of the parts doesn't have a Sympy version - presumably there is an error message, but is it a useful one?

It fails with an unpleasant error message because SymPy tries too hard to make sense of it. Example:

sage: F = FiniteEnumeratedSets().example()
sage: cartesian_product([F, F])._sympy_()
SymPyDeprecationWarning: 

String fallback in sympify has been deprecated since SymPy 1.6. Use
sympify(str(obj)) or sympy.core.sympify.converter or obj._sympy_
instead. See https://github.com/sympy/sympy/issues/18066 for more
info.

  SymPyDeprecationWarning(.....)

SyntaxError: invalid syntax (<string>, line 1)

During handling of the above exception, another exception occurred:

SympifyError: Sympify of expression 'could not parse 'An example of a finite enumerated set: {1,2,3}'' failed, because of exception being raised:
SyntaxError: invalid syntax (<string>, line 1)

#31938 will provide all Sage sets with a _sympy_ method.

@kcrisman
Copy link
Member

kcrisman commented Jun 9, 2021

comment:6

Replying to @mkoeppe:

Replying to @kcrisman:

with the products, what happens if one of the parts doesn't have a Sympy version - presumably there is an error message, but is it a useful one?

It fails with an unpleasant error message because SymPy tries too hard to make sense of it. Example:

I agree this isn't as helpful to the end user. What I'd formally recommend as a review, until #31938 is merged, is to document this type of error as a doctest so it is at least searchable if someone comes across it.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 9, 2021

Dependencies: #31938

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 9, 2021

comment:8

Now, after merging #31938, this example works:

sage: F = FiniteEnumeratedSets().example()
sage: cartesian_product([F, F])._sympy_()
ProductSet(SageSet(An example of a finite enumerated set: {1,2,3}), SageSet(An example of a finite enumerated set: {1,2,3}))

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 10, 2021

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

93fbb2bCall sympy_init in all added `_sympy_` methods
6e5cac6sage.interfaces.sympy_wrapper, Sets.ParentMethods._sympy_: New
3cac256sage.interfaces.sympy_wrapper: Add doctests
eef604eSageSet: Finish docstrings; handle symbolic _contains
2baae58Sets.ParentMethods._sympy_: Call sympy_init
153b3e5Merge #31938

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 10, 2021

Changed commit from 9fcf32e to 153b3e5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 10, 2021

Changed commit from 153b3e5 to f535127

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 10, 2021

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

c06c965sage.interfaces.sympy_wrapper.SageSet: Add another doctest
f535127Merge #31938

@kcrisman
Copy link
Member

comment:11

Now, after merging #31938, this example works:

Nice. I am not reviewing that one, but anyway that was my only comment on this one.

@tscrim
Copy link
Collaborator

tscrim commented Jun 10, 2021

comment:12

@kcrisman Will you be reviewing this ticket? I have basically reviewed #31938; just waiting on patchbot approval.

@kcrisman
Copy link
Member

comment:13

@kcrisman Will you be reviewing this ticket? I have basically reviewed #31938; just waiting on patchbot approval.

I'm happy with this one given your review of that one, as long as patchbot is. I am unable to manually test however, so your additional green light would be great.

@tscrim
Copy link
Collaborator

tscrim commented Jun 10, 2021

comment:14
sage -t --long --random-seed=0 src/sage/sets/real_set.py  # 7 doctests failed

because there is no ambient() method called via the is_universe() method.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2021

Changed commit from f535127 to 32cdd5c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2021

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

4b09685RealSet: Put it in a suitable subcategory of TopologicalSpaces()
46eed0eRealSet.ambient: Change to a normal method
32cdd5cMerge #31877

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 11, 2021

Changed dependencies from #31938 to #31938, #31877

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 11, 2021

comment:16

Right, that method comes from #31877, which I have now merged

@tscrim
Copy link
Collaborator

tscrim commented Jun 11, 2021

comment:17

Now to wait for the patchbot one more time.

@tscrim
Copy link
Collaborator

tscrim commented Jun 11, 2021

Reviewer: Karl-Dieter Crisman

@slel
Copy link
Member

slel commented Jun 11, 2021

comment:18

Patchbots seem stuck on #31928,
see #31928 comment:1.

@tscrim
Copy link
Collaborator

tscrim commented Jun 12, 2021

comment:19

Setting to positive as per comment:13.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 12, 2021

comment:20

Thanks!

@vbraun
Copy link
Member

vbraun commented Jun 29, 2021

Changed branch from u/mkoeppe/_sympy__methods_for_some_parent_classes to 32cdd5c

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

5 participants