#32015 closed enhancement (fixed)

_sympy_ methods for Set_object_binary subclasses

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.4
Component: interfaces Keywords:
Cc: tscrim, kcrisman, egourgoulhon 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:

Status badges

Description (last modified by mkoeppe)

Formal unions, intersections, ...

Change History (26)

comment:1 Changed 13 months ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 13 months ago by mkoeppe

  • Branch set to u/mkoeppe/_sympy__methods_for_set_object_binary_subclasses

comment:3 Changed 13 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Cc kcrisman added
  • Commit set to 3dd50e485860e39ce19973eba58fc9f0ed43ba0b
  • Status changed from new to needs_review

Last 10 new commits:

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
c06c965sage.interfaces.sympy_wrapper.SageSet: Add another doctest
f535127Merge #31938
32cdd5cMerge #31877
789dc05Merge #31931
3dd50e4sage.sets.set.Set_object: Add _sympy_ methods to subclasses

comment:4 Changed 12 months ago by mkoeppe

  • Cc egourgoulhon added
  • Dependencies changed from #32013, #31931, #31938 to #32013

comment:5 Changed 12 months ago by git

  • Commit changed from 3dd50e485860e39ce19973eba58fc9f0ed43ba0b to 7f35aee31a94d4c22ce604b6e5ba7ec34c260720

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

200b1efMerge tag '9.4.beta4' into t/32013/initialize_a_set_from_a_convexset_base_instance
7f35aeeMerge #32013

comment:6 Changed 12 months ago by tscrim

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 12 months ago by mkoeppe

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 12 months ago by tscrim

  • Reviewers set to 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 12 months ago by mkoeppe

  • Status changed from needs_review to positive_review

Thanks! I'm keeping it as is.

comment:10 Changed 12 months ago by vbraun

  • Status changed from positive_review to 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 12 months ago by git

  • Commit changed from 7f35aee31a94d4c22ce604b6e5ba7ec34c260720 to daeb91e7313d4240002e573078fb5547cc3a8a78

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

daeb91esrc/sage/sets/set.py: Fix docstring markup

comment:12 Changed 12 months ago by mkoeppe

  • Status changed from needs_work to positive_review

comment:13 Changed 12 months ago by vbraun

  • Status changed from positive_review to needs_work

Tests fail

comment:14 Changed 12 months ago by git

  • Commit changed from daeb91e7313d4240002e573078fb5547cc3a8a78 to 26824697667f2eec0977b532f442e8e2ed4a8058

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

2682469src/sage/interfaces/sympy_wrapper.py: Use Family, not Set, in doctests to make sure that the SageSet wrapper is actually used

comment:15 Changed 12 months ago by git

  • Commit changed from 26824697667f2eec0977b532f442e8e2ed4a8058 to 753babb46c7bbe70faa1e0da4d7887b4fa5867e0

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

753babbSet_object_enumerated._sympy_: Translate empty sets to EmptySet

comment:16 Changed 12 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:17 Changed 12 months ago by tscrim

  • Status changed from needs_review to positive_review

LGTM.

comment:18 Changed 12 months ago by gh-mjungmath

  • Status changed from positive_review to 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/site-packages/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/site-packages/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 12 months ago by git

  • Commit changed from 753babb46c7bbe70faa1e0da4d7887b4fa5867e0 to 451f5cf5fb60611db32bf5fbcdecfec230c087f6

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

451f5cfSets.ParentMethods: Update doctest

comment:20 Changed 12 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:21 Changed 12 months ago by tscrim

  • Status changed from needs_review to positive_review

Thanks for catching that.

comment:22 Changed 12 months ago by gh-mjungmath

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Michael Jung
sage -t --random-seed=0 src/sage/categories/sets_cat.py
    [478 tests, 0.52 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

comment:23 Changed 12 months ago by mkoeppe

Thanks!

comment:24 Changed 12 months ago by git

  • Commit changed from 451f5cf5fb60611db32bf5fbcdecfec230c087f6 to 689986b2a5f5d9b839af7c0b1dd13910d20cb928
  • Status changed from positive_review to needs_review

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

63c2cfeConvexSet_base._test_convex_set: Do not test _test_as_set_object here
55240bbMerge #30473
fff2a79src/sage/sets/set.py: Fix docstring markup
70a2e7dMerge #32013
689986bMerge tag '9.4.beta5' into t/32015/_sympy__methods_for_set_object_binary_subclasses

comment:25 Changed 12 months ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:26 Changed 11 months ago by vbraun

  • Branch changed from u/mkoeppe/_sympy__methods_for_set_object_binary_subclasses to 689986b2a5f5d9b839af7c0b1dd13910d20cb928
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.