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

Various Cython fixes to IndexedFreeModuleElement #23440

Closed
jdemeyer opened this issue Jul 15, 2017 · 8 comments
Closed

Various Cython fixes to IndexedFreeModuleElement #23440

jdemeyer opened this issue Jul 15, 2017 · 8 comments

Comments

@jdemeyer
Copy link

  1. __div__ = __truediv__ is a bad idea for extension types because it acts on the Python method level and not on the slot level:
sage: F = CombinatorialFreeModule(QQ, [1,2,3]); B = F.basis(); f = 2*B[2] + 4*B[3]
sage: op = operator.truediv; timeit('op(f, 2)', repeat=100000, number=20)
20 loops, best of 100000: 4.99 µs per loop
sage: op = operator.div; timeit('op(f, 2)', repeat=100000, number=20)
20 loops, best of 100000: 5.45 µs per loop
  1. IndexedFreeModuleElement and LieAlgebraElement implement arithmetic methods as cdef _add_ while every other class in Sage uses cpdef _add_.

  2. six is slow and should be avoided.

  3. IndexedFreeModuleElement must inherit from ModuleElement. This is because it uses the cpdef _lmul_ and cpdef _rmul_ which are provided by ModuleElement but not by Element. This causes trouble for FreeAlgebraElement which inherits from IndexedFreeModuleElement and AlgebraElement.

CC: @tscrim @nthiery

Component: cython

Author: Jeroen Demeyer

Branch/Commit: 8120246

Reviewer: Travis Scrimshaw

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

@jdemeyer jdemeyer added this to the sage-8.1 milestone Jul 15, 2017
@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

@jdemeyer
Copy link
Author

New commits:

8120246Various Cython fixes

@jdemeyer
Copy link
Author

Commit: 8120246

@tscrim
Copy link
Collaborator

tscrim commented Jul 18, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jul 18, 2017

comment:5

LGTM. Thank you for cleaning it up. The last part is also useful for #22591 (although I think there is more of a shortcomming of *ModuleAction in play, but this works around the problem).

@vbraun
Copy link
Member

vbraun commented Jul 26, 2017

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

3 participants