Opened 18 months ago
Closed 17 months ago
#32015 closed enhancement (fixed)
_sympy_ methods for Set_object_binary subclasses
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  interfaces  Keywords:  
Cc:  Travis Scrimshaw, KarlDieter Crisman, Eric Gourgoulhon  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Travis Scrimshaw, Michael Jung 
Report Upstream:  N/A  Work issues:  
Branch:  689986b (Commits, GitHub, GitLab)  Commit:  689986b2a5f5d9b839af7c0b1dd13910d20cb928 
Dependencies:  #32013  Stopgaps: 
Description (last modified by )
Formal unions, intersections, ...
Change History (26)
comment:1 Changed 18 months ago by
Description:  modified (diff) 

comment:2 Changed 18 months ago by
Branch:  → u/mkoeppe/_sympy__methods_for_set_object_binary_subclasses 

comment:3 Changed 18 months ago by
Authors:  → Matthias Koeppe 

Cc:  KarlDieter Crisman added 
Commit:  → 3dd50e485860e39ce19973eba58fc9f0ed43ba0b 
Status:  new → needs_review 
comment:4 Changed 17 months ago by
Cc:  Eric Gourgoulhon added 

Dependencies:  #32013, #31931, #31938 → #32013 
comment:5 Changed 17 months ago by
Commit:  3dd50e485860e39ce19973eba58fc9f0ed43ba0b → 7f35aee31a94d4c22ce604b6e5ba7ec34c260720 

comment:6 Changed 17 months ago by
I am not sure about caching the output of _sympy_
? It creates a clone of the set and ties it to the Sage object. Can you give some reasoning for this?
comment:7 Changed 17 months ago by
If one makes one computation with the sympy version of a set, it's likely that another computation will be made  so saving the result seemed a good idea.
comment:8 Changed 17 months ago by
Reviewers:  → Travis Scrimshaw 

I would expect someone to store said Sympy version if they were going to use it again. Granted, I expect this to not be a problem either way. My instinct is to err on the side of not caching, but I don't have a strong opinion on this. So you either change it or not; once decided, you can set a positive review.
comment:9 Changed 17 months ago by
Status:  needs_review → positive_review 

Thanks! I'm keeping it as is.
comment:10 Changed 17 months ago by
Status:  positive_review → needs_work 

[docpdf] [docpdf] Overfull \hbox (22.51231p [docpdf] ! Missing { inserted. [docpdf] <to be read again> [docpdf] _ [docpdf] l.1488 ...n{}in class providing the operators \(__ [docpdf] add__\) and \(__sub__\). [docpdf] ? [docpdf] [809 [docpdf] ! Emergency stop. [docpdf] <to be read again> [docpdf] _ [docpdf] l.1488 ...n{}in class providing the operators \(__ [docpdf] add__\) and \(__sub__\). [docpdf] ! ==> Fatal error occurred, no output PDF file produced!
comment:11 Changed 17 months ago by
Commit:  7f35aee31a94d4c22ce604b6e5ba7ec34c260720 → daeb91e7313d4240002e573078fb5547cc3a8a78 

Branch pushed to git repo; I updated commit sha1. New commits:
daeb91e  src/sage/sets/set.py: Fix docstring markup

comment:12 Changed 17 months ago by
Status:  needs_work → positive_review 

comment:14 Changed 17 months ago by
Commit:  daeb91e7313d4240002e573078fb5547cc3a8a78 → 26824697667f2eec0977b532f442e8e2ed4a8058 

Branch pushed to git repo; I updated commit sha1. New commits:
2682469  src/sage/interfaces/sympy_wrapper.py: Use Family, not Set, in doctests to make sure that the SageSet wrapper is actually used

comment:15 Changed 17 months ago by
Commit:  26824697667f2eec0977b532f442e8e2ed4a8058 → 753babb46c7bbe70faa1e0da4d7887b4fa5867e0 

Branch pushed to git repo; I updated commit sha1. New commits:
753babb  Set_object_enumerated._sympy_: Translate empty sets to EmptySet

comment:16 Changed 17 months ago by
Status:  needs_work → needs_review 

comment:18 Changed 17 months ago by
Status:  positive_review → needs_work 

File "src/sage/categories/sets_cat.py", line 1720, in sage.categories.sets_cat.Sets.ParentMethods._sympy_ Failed example: sF = F._sympy_(); sF Expected: SageSet({1, 2}) Got: Set(1, 2)
and
File "src/sage/categories/sets_cat.py", line 1722, in sage.categories.sets_cat.Sets.ParentMethods._sympy_ Failed example: sF._sage_() is F Exception raised: Traceback (most recent call last): File "/data/pell/lipnik/patchbot/local/lib/python3.9/sitepackages/sage/doctest/forker.py", line 718, in _run self.compile_and_execute(example, compiler, test.globs) File "/data/pell/lipnik/patchbot/local/lib/python3.9/sitepackages/sage/doctest/forker.py", line 1137, in compile_and_execute exec(compiled, globs) File "<doctest sage.categories.sets_cat.Sets.ParentMethods._sympy_[12]>", line 1, in <module> sF._sage_() is F AttributeError: 'Set' object has no attribute '_sage_'
comment:19 Changed 17 months ago by
Commit:  753babb46c7bbe70faa1e0da4d7887b4fa5867e0 → 451f5cf5fb60611db32bf5fbcdecfec230c087f6 

Branch pushed to git repo; I updated commit sha1. New commits:
451f5cf  Sets.ParentMethods: Update doctest

comment:20 Changed 17 months ago by
Status:  needs_work → needs_review 

comment:21 Changed 17 months ago by
Status:  needs_review → positive_review 

Thanks for catching that.
comment:22 Changed 17 months ago by
Reviewers:  Travis Scrimshaw → Travis Scrimshaw, Michael Jung 

sage t randomseed=0 src/sage/categories/sets_cat.py [478 tests, 0.52 s]  All tests passed! 
comment:24 Changed 17 months ago by
Commit:  451f5cf5fb60611db32bf5fbcdecfec230c087f6 → 689986b2a5f5d9b839af7c0b1dd13910d20cb928 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
63c2cfe  ConvexSet_base._test_convex_set: Do not test _test_as_set_object here

55240bb  Merge #30473

fff2a79  src/sage/sets/set.py: Fix docstring markup

70a2e7d  Merge #32013

689986b  Merge tag '9.4.beta5' into t/32015/_sympy__methods_for_set_object_binary_subclasses

comment:25 Changed 17 months ago by
Status:  needs_review → positive_review 

comment:26 Changed 17 months ago by
Branch:  u/mkoeppe/_sympy__methods_for_set_object_binary_subclasses → 689986b2a5f5d9b839af7c0b1dd13910d20cb928 

Resolution:  → fixed 
Status:  positive_review → closed 
Last 10 new commits:
sage.interfaces.sympy_wrapper, Sets.ParentMethods._sympy_: New
sage.interfaces.sympy_wrapper: Add doctests
SageSet: Finish docstrings; handle symbolic _contains
Sets.ParentMethods._sympy_: Call sympy_init
Merge #31938
sage.interfaces.sympy_wrapper.SageSet: Add another doctest
Merge #31938
Merge #31877
Merge #31931
sage.sets.set.Set_object: Add _sympy_ methods to subclasses