Opened 7 years ago

Closed 7 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)

trac_14519-cynthonize_element_wrapper-ts.patch (88.1 KB) - added by tscrim 7 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by tscrim

  • Dependencies set to #14143 #14516
  • Status changed from new to needs_review

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.

comment:2 Changed 7 years ago by tscrim

  • Status changed from needs_review to needs_work
  • Work issues set to fix segfault in UCF

comment:3 Changed 7 years ago by tscrim

  • 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 7 years ago 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.

comment:5 Changed 7 years ago by tscrim

This is likely the same issue; luckily I could work (hack) around it here.

comment:6 Changed 7 years ago 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.

comment:7 Changed 7 years ago by tscrim

  • 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

comment:8 Changed 7 years ago 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,
Travis

comment:9 Changed 7 years ago by nthiery

  • Reviewers set to Nicolas M. Thiéry
  • Status changed from needs_review to positive_review

comment:10 Changed 7 years ago by tscrim

Thanks for doing the review Nicolas.

comment:11 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:12 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.12 to sage-pending

comment:13 Changed 7 years ago by tscrim

  • Milestone changed from sage-pending to sage-5.12

comment:14 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.12.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.