Opened 9 years ago
Closed 9 years ago
#14519 closed enhancement (fixed)
Cythonize ElementWrapper and make parent the first argument
Reported by: | tscrim | Owned by: | tscrim |
---|---|---|---|
Priority: | major | Milestone: | sage-5.12 |
Component: | performance | Keywords: | cython ElementWrapper |
Cc: | sage-combinat, nthiery, SimonKing | Merged in: | sage-5.12.beta2 |
Authors: | Travis Scrimshaw | Reviewers: | Nicolas M. Thiéry |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #14143 #14015 #14516 | Stopgaps: |
Description
For speed and consistency.
Attachments (1)
Change History (15)
comment:1 Changed 9 years ago by
- Dependencies set to #14143 #14516
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Status changed from needs_review to needs_work
- Work issues set to fix segfault in UCF
comment:3 Changed 9 years ago by
- Status changed from needs_work to needs_review
- Work issues fix segfault in UCF deleted
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.
comment:4 Changed 9 years ago by
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.
comment:5 Changed 9 years ago by
This is likely the same issue; luckily I could work (hack) around it here.
comment:6 Changed 9 years ago by
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.
comment:7 Changed 9 years ago by
- Dependencies changed from #14143 #14516 to #14143 #14015 #14516
Hey Nicolas,
Thank you for reviewing the patch.
Replying to 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,
Travis
Changed 9 years ago by
comment:8 Changed 9 years ago by
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,
Travis
comment:9 Changed 9 years ago by
- Reviewers set to Nicolas M. Thiéry
- Status changed from needs_review to positive_review
comment:10 Changed 9 years ago by
Thanks for doing the review Nicolas.
comment:11 Changed 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:12 Changed 9 years ago by
- Milestone changed from sage-5.12 to sage-pending
comment:13 Changed 9 years ago by
- Milestone changed from sage-pending to sage-5.12
comment:14 Changed 9 years ago by
- Merged in set to sage-5.12.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
Initial version which has a dependencies on #14143 (minor reject) and #14516 (since
Letter
no longer inherits fromElementWrapper
, there is nothing to do there). Both of which this can be moved past with minor-moderate cost.