Cythonize ElementWrapper and make parent the first argument

[tscrim]

For speed and consistency.

Depends on #14143
Depends on #14015
Depends on #14516

CC: @sagetrac-sage-combinat @nthiery @simon-king-jena

Component: performance

Keywords: cython ElementWrapper

Author: Travis Scrimshaw

Reviewer: Nicolas M. Thiéry

Merged: sage-5.12.beta2

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

[tscrim]

Dependencies: #14143 #14516

[tscrim]

comment:1

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.

[tscrim]

Work Issues: fix segfault in UCF

[tscrim]

comment:3

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.

[tscrim]

Changed work issues from fix segfault in UCF to none

[simon-king-jena]

comment:4

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.

[tscrim]

comment:5

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

[nthiery]

comment:6

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.

[tscrim]

comment:7

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

[tscrim]

Changed dependencies from #14143 #14516 to #14143 #14015 #14516

[tscrim]

Attachment: trac_14519-cynthonize_element_wrapper-ts.patch.gz

[tscrim]

comment:8

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

[nthiery]

Reviewer: Nicolas M. Thiéry

[tscrim]

comment:10

Thanks for doing the review Nicolas.

[jdemeyer]

Merged: sage-5.12.beta2