[Sage] #14519: Cythonize ElementWrapper and make parent the first argument
Sage
---------------------------+------------------------------------------------
Reporter: tscrim | Owner: tscrim
Type: enhancement | Status: new
Priority: major | Milestone: sage-5.10
Component: performance | Keywords: cython ElementWrapper
Work issues: | Report Upstream: N/A
Reviewers: | Authors: Travis Scrimshaw
Merged in: | Dependencies:
Stopgaps: |
---------------------------+------------------------------------------------
For speed and consistency.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica, and MATLAB
Sage
Reporter: tscrim | Owner: tscrim
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.10
Component: performance | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
-----------------------------------------+----------------------------------
Changes (by tscrim):
* status: new => needs_review
* dependencies: => #14143 #14516
Comment:
Initial version which has a dependencies on #14143 (minor reject) and
#14516 (since `Letter` no longer inherits from `ElementWrapper`, there is
nothing to do there). Both of which this can be moved past with minor-
moderate cost.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519#comment:1>
Sage
Reporter: tscrim | Owner: tscrim
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-5.10
Component: performance | Resolution:
Keywords: cython ElementWrapper | Work issues: fix segfault in UCF
Authors: Travis Scrimshaw | Merged in:
-----------------------------------------+----------------------------------
Changes (by tscrim):
* status: needs_review => needs_work
* work_issues: => fix segfault in UCF
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519#comment:2>
Sage
Reporter: tscrim | Owner: tscrim
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: performance | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
-----------------------------------------+----------------------------------
Changes (by tscrim):
* status: needs_work => needs_review
* work_issues: fix segfault in UCF =>
Comment:
The UCF segfault was due to it inheriting from both `FieldElement` and
`ElementWrapper`. In particular, the `_add_()` / `_mul_()` / etc.
functions of the UCF elements were not being called when `ElementWrapper`
is an extension class. It also segfaults when I have it inherit from
`CombinatorialFreeModuleElement`, which is what it wraps. I'll see if I
can reduce it down to a simple test case and post it on another ticket and
sage-devel since I don't know if this is a python, cython, or sage bug...
Also #11931 can be closed as a duplicate.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519#comment:3>
Sage
Reporter: tscrim | Owner: tscrim
Priority: major | Milestone: sage-5.11
Component: performance | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
-----------------------------------------+----------------------------------
Comment (by SimonKing):
Note that I did experience a very subtle Cython problem: When one
constructs a Python class that simultaneously inherits from `RingElement`
and `Morphism`, then the two bases seem to have a compatible layout,
however two ''different'' cpdef attributes of the two bases get confused.
Perhaps this is similar here?
Sorry, I am too lazy to look up the ticket number or the discussion on the
cython-user mailing list.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519#comment:4>
Sage
Reporter: tscrim | Owner: tscrim
Priority: major | Milestone: sage-5.11
Component: performance | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
-----------------------------------------+----------------------------------
Comment (by tscrim):
This is likely the same issue; luckily I could work (hack) around it here.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519#comment:5>
Sage
Reporter: tscrim | Owner: tscrim
Priority: major | Milestone: sage-5.11
Component: performance | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
-----------------------------------------+----------------------------------
Comment (by nthiery):
Hi Travis,
I am finally looking at the patch now. Thanks for your work on this!
Some points:
- In those places where the patch touches a "..." continuation, use the
occasion to make it a "....:"
- Line 312 of the coercion tutorial: missing space before D (it was
missing already)
- In all the calls to MyElement: proper spacing between arguments
- universal_cyclotomic_field.py:
- Unless unavoidable, don't change the order of the imports, and keep
CombinatorialFreeModule imported in the __init__ instead of in the module.
- Mention in the code that UCFElement can't multiple inherit from the
two extension types FieldElement and ElementWrapper, which is why at this
point the methods _latex_ and friends have to be duplicated (in the long
run we can hope not to need anymore to inherit from FieldElement).
- Check the spacing between the arguments in the call to element_class
- disjoint_union_enumerated_sets.py: why did the type of ``el`` change?
- The __nonzero__ feature is new, right? I'd rather avoid it at this
point, since the semantic of being zero might differ quite some depending
on the use case.
Let me know when you will have posted an updated patch fixing those as
well as the failing tests detected by the patchbot.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519#comment:6>
Sage
Reporter: tscrim | Owner: tscrim
Priority: major | Milestone: sage-5.11
Component: performance | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
-----------------------------------------+----------------------------------
Changes (by tscrim):
* dependencies: #14143 #14516 => #14143 #14015 #14516
Comment:
Hey Nicolas,
Thank you for reviewing the patch.
Replying to [comment:6 nthiery]:
> Some points:
>
> - In those places where the patch touches a "..." continuation, use the
occasion to make it a "....:"
> - Line 312 of the coercion tutorial: missing space before D (it was
missing already)
> - In all the calls to MyElement: proper spacing between arguments
> - universal_cyclotomic_field.py:
> - Unless unavoidable, don't change the order of the imports, and keep
CombinatorialFreeModule imported in the __init__ instead of in the module.
> - Mention in the code that UCFElement can't multiple inherit from the
two extension types FieldElement and ElementWrapper, which is why at this
point the methods _latex_ and friends have to be duplicated (in the long
run we can hope not to need anymore to inherit from FieldElement).
Done.
> - Check the spacing between the arguments in the call to element_class
I think done; if not I'll need something more specific.
> - disjoint_union_enumerated_sets.py: why did the type of ``el`` change?
Because it because an extension class.
> - The __nonzero__ feature is new, right? I'd rather avoid it at this
point, since the semantic of being zero might differ quite some depending
on the use case.
Done.
> Let me know when you will have posted an updated patch fixing those as
well as the failing tests detected by the patchbot.
The updated patch fixes almost everything except 1 doctest in
`misc/nested_class_test.py` (as opposed to 2 previously) and I'm not quite
sure what the problem is currently. I don't think I need a `__reduce__()`
in the element wrapper since it has a `__dict__` (and my `__reduce__()`
was causing pickling to fail). If you have any insight into this, I'd
appreciate it.
Thanks,[[BR]]
Travis
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519#comment:7>
Sage
Reporter: tscrim | Owner: tscrim
Priority: major | Milestone: sage-5.11
Component: performance | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
-----------------------------------------+----------------------------------
Comment (by tscrim):
Okay, I figured out what was going wrong. For the nested class,
`TestParent4` did not implement a `__ne__` and is not a
`UniqueRepresentation`, so it does not by default check `not __eq__`. In
the `ElementWrapper`, I'm testing using `__ne__`.
For the posets, the problem was that `ElementWrapper` was checking if the
second argument was a `Parent` class, rather than if the first argument is
not a `Parent`. Thus wrapping a `Set`, which is a `Parent`, caused the
deprecation warning and a swap to occur.
The rest of the errors were trivial in the sense I needed to put a parent
as the first argument. In summary: err0rz b3 pwnd.
Best,[[BR]]
Travis
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519#comment:8>
Sage
Reporter: tscrim | Owner: tscrim
Type: enhancement | Status: positive_review
Component: performance | Resolution:
-----------------------------------------+----------------------------------
* status: needs_review => positive_review
* reviewer: => Nicolas M. Thiéry
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519#comment:9>
Sage
Reporter: tscrim | Owner: tscrim
Priority: major | Milestone: sage-5.11
Component: performance | Resolution:
-----------------------------------------+----------------------------------
Comment (by tscrim):
Thanks for doing the review Nicolas.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14519#comment:10>