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 Set_object_binary subclasses #32015

Closed
mkoeppe opened this issue Jun 20, 2021 · 32 comments
Closed

_sympy_ methods for Set_object_binary subclasses #32015

mkoeppe opened this issue Jun 20, 2021 · 32 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 20, 2021

Formal unions, intersections, ...

Depends on #32013

CC: @tscrim @kcrisman @egourgoulhon

Component: interfaces

Author: Matthias Koeppe

Branch/Commit: 689986b

Reviewer: Travis Scrimshaw, Michael Jung

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

@mkoeppe mkoeppe added this to the sage-9.4 milestone Jun 20, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 21, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 21, 2021

Commit: 3dd50e4

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 21, 2021

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 21, 2021

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 1, 2021

Changed dependencies from #32013, #31931, #31938 to #32013

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2021

Changed commit from 3dd50e4 to 7f35aee

@tscrim
Copy link
Collaborator

tscrim commented Jul 2, 2021

comment:6

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?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 2, 2021

comment:7

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.

@tscrim
Copy link
Collaborator

tscrim commented Jul 2, 2021

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jul 2, 2021

comment:8

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 2, 2021

comment:9

Thanks! I'm keeping it as is.

@vbraun
Copy link
Member

vbraun commented Jul 8, 2021

comment:10
[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!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2021

Changed commit from 7f35aee to daeb91e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2021

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

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

@vbraun
Copy link
Member

vbraun commented Jul 9, 2021

comment:13

Tests fail

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2021

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

753babbSet_object_enumerated._sympy_: Translate empty sets to EmptySet

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2021

Changed commit from 2682469 to 753babb

@tscrim
Copy link
Collaborator

tscrim commented Jul 10, 2021

comment:17

LGTM.

@mjungmath
Copy link

comment:18
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_'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 13, 2021

Changed commit from 753babb to 451f5cf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 13, 2021

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

451f5cfSets.ParentMethods: Update doctest

@tscrim
Copy link
Collaborator

tscrim commented Jul 13, 2021

comment:21

Thanks for catching that.

@mjungmath
Copy link

comment:22
sage -t --random-seed=0 src/sage/categories/sets_cat.py
    [478 tests, 0.52 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

@mjungmath
Copy link

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Michael Jung

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 13, 2021

comment:23

Thanks!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 19, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 19, 2021

Changed commit from 451f5cf to 689986b

@vbraun
Copy link
Member

vbraun commented Jul 23, 2021

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

4 participants