#31931 closed enhancement (fixed)

_sympy_ methods for some parent classes

Reported by: Matthias Köppe Owned by:
Priority: major Milestone: sage-9.4
Component: interfaces Keywords:
Cc: Travis Scrimshaw, Karl-Dieter Crisman, Ondřej Čertík, Vincent Delecroix Merged in:
Authors: Matthias Koeppe Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: 32cdd5c (Commits, GitHub, GitLab) Commit: 32cdd5c1c8db52696499212f4b27d53579d48163
Dependencies: #31938, #31877 Stopgaps:

Status badges

Description (last modified by Matthias Köppe)

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

Change History (21)

comment:1 Changed 16 months ago by Matthias Köppe

Branch: u/mkoeppe/_sympy__methods_for_some_parent_classes

comment:2 Changed 16 months ago by Matthias Köppe

Authors: Matthias Koeppe
Cc: Travis Scrimshaw Karl-Dieter Crisman Ondřej Čertík Vincent Delecroix added
Commit: 9fcf32ea544d7e841a41deda364c179205108e9b
Status: newneeds_review

New commits:

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

comment:3 Changed 16 months ago by Karl-Dieter Crisman

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?

comment:4 in reply to:  3 Changed 16 months ago by Matthias Köppe

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.

comment:5 in reply to:  3 ; Changed 16 months ago by Matthias Köppe

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.

comment:6 in reply to:  5 Changed 16 months ago by Karl-Dieter Crisman

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.

comment:7 Changed 16 months ago by Matthias Köppe

Dependencies: #31938
Description: modified (diff)

comment:8 Changed 16 months ago by Matthias Köppe

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

comment:9 Changed 16 months ago by git

Commit: 9fcf32ea544d7e841a41deda364c179205108e9b153b3e5fdca2bbbb59e2372873c7c7afde2a0104

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

comment:10 Changed 16 months ago by git

Commit: 153b3e5fdca2bbbb59e2372873c7c7afde2a0104f53512772358d7f42f9e06e306b5c490d2833a33

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

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

comment:11 in reply to:  8 Changed 16 months ago by Karl-Dieter Crisman

Now, after merging #31938, this example works:

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

comment:12 Changed 16 months ago by Travis Scrimshaw

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

comment:13 in reply to:  12 Changed 16 months ago by Karl-Dieter Crisman

@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.

comment:14 Changed 16 months ago by Travis Scrimshaw

Status: needs_reviewneeds_work
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.

comment:15 Changed 16 months ago by git

Commit: f53512772358d7f42f9e06e306b5c490d2833a3332cdd5c1c8db52696499212f4b27d53579d48163

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

comment:16 Changed 16 months ago by Matthias Köppe

Dependencies: #31938#31938, #31877
Status: needs_workneeds_review

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

comment:17 Changed 16 months ago by Travis Scrimshaw

Reviewers: Karl-Dieter Crisman

Now to wait for the patchbot one more time.

comment:18 Changed 16 months ago by Samuel Lelièvre

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

comment:19 Changed 16 months ago by Travis Scrimshaw

Status: needs_reviewpositive_review

Setting to positive as per comment:13.

Last edited 16 months ago by Travis Scrimshaw (previous) (diff)

comment:20 Changed 16 months ago by Matthias Köppe

Thanks!

comment:21 Changed 15 months ago by Volker Braun

Branch: u/mkoeppe/_sympy__methods_for_some_parent_classes32cdd5c1c8db52696499212f4b27d53579d48163
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.