Opened 16 months ago
Closed 15 months ago
#31931 closed enhancement (fixed)
_sympy_ methods for some parent classes
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  interfaces  Keywords:  
Cc:  Travis Scrimshaw, KarlDieter Crisman, Ondřej Čertík, Vincent Delecroix  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  KarlDieter Crisman 
Report Upstream:  N/A  Work issues:  
Branch:  32cdd5c (Commits, GitHub, GitLab)  Commit:  32cdd5c1c8db52696499212f4b27d53579d48163 
Dependencies:  #31938, #31877  Stopgaps: 
Description (last modified by )
Change History (21)
comment:1 Changed 16 months ago by
Branch:  → u/mkoeppe/_sympy__methods_for_some_parent_classes 

comment:2 Changed 16 months ago by
Authors:  → Matthias Koeppe 

Cc:  Travis Scrimshaw KarlDieter Crisman Ondřej Čertík Vincent Delecroix added 
Commit:  → 9fcf32ea544d7e841a41deda364c179205108e9b 
Status:  new → needs_review 
comment:3 followups: 4 5 Changed 16 months ago by
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 Changed 16 months ago by
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 followup: 6 Changed 16 months ago by
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 Changed 16 months ago by
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
Dependencies:  → #31938 

Description:  modified (diff) 
comment:8 followup: 11 Changed 16 months ago by
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
Commit:  9fcf32ea544d7e841a41deda364c179205108e9b → 153b3e5fdca2bbbb59e2372873c7c7afde2a0104 

Branch pushed to git repo; I updated commit sha1. New commits:
93fbb2b  Call sympy_init in all added _sympy_ methods

6e5cac6  sage.interfaces.sympy_wrapper, Sets.ParentMethods._sympy_: New

3cac256  sage.interfaces.sympy_wrapper: Add doctests

eef604e  SageSet: Finish docstrings; handle symbolic _contains

2baae58  Sets.ParentMethods._sympy_: Call sympy_init

153b3e5  Merge #31938

comment:10 Changed 16 months ago by
Commit:  153b3e5fdca2bbbb59e2372873c7c7afde2a0104 → f53512772358d7f42f9e06e306b5c490d2833a33 

comment:11 Changed 16 months ago by
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 followup: 13 Changed 16 months ago by
@kcrisman Will you be reviewing this ticket? I have basically reviewed #31938; just waiting on patchbot approval.
comment:13 Changed 16 months ago by
@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
Status:  needs_review → needs_work 

sage t long randomseed=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
Commit:  f53512772358d7f42f9e06e306b5c490d2833a33 → 32cdd5c1c8db52696499212f4b27d53579d48163 

comment:16 Changed 16 months ago by
Dependencies:  #31938 → #31938, #31877 

Status:  needs_work → needs_review 
Right, that method comes from #31877, which I have now merged
comment:17 Changed 16 months ago by
Reviewers:  → KarlDieter Crisman 

Now to wait for the patchbot one more time.
comment:19 Changed 16 months ago by
Status:  needs_review → positive_review 

Setting to positive as per comment:13.
comment:21 Changed 15 months ago by
Branch:  u/mkoeppe/_sympy__methods_for_some_parent_classes → 32cdd5c1c8db52696499212f4b27d53579d48163 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Sets.CartesianProducts.ParentMethods, FreeModule_ambient, IntegerRing_class, InternalRealInterval, RealSet, NonNegativeIntegers, IntegerRing_class, PositiveIntegers, RationalField: Add _sympy_ methods