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

Fix Cython warnings in finite_rings #24116

Closed
jdemeyer opened this issue Oct 27, 2017 · 13 comments
Closed

Fix Cython warnings in finite_rings #24116

jdemeyer opened this issue Oct 27, 2017 · 13 comments

Comments

@jdemeyer
Copy link

Fix all warnings "Compatible but non-identical C method ... not redeclared" and "Overriding cdef method with def method"

This is part of #23600

Component: cython

Author: Jeroen Demeyer

Branch: 9dbd211

Reviewer: Travis Scrimshaw

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

@jdemeyer jdemeyer added this to the sage-8.1 milestone Oct 27, 2017
@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

@jdemeyer
Copy link
Author

New commits:

9dbd211Add abstract `_add_` and `_mul_` methods to FiniteRingElement

@jdemeyer
Copy link
Author

Commit: 9dbd211

@jdemeyer

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Oct 29, 2017

comment:5

Can you explain a little bit why you need to explicitly add an _add_ and _mul_ methods to FiniteRingElement? It seems like it is not (suppose to be) a concrete class and we do not do similar declarations in our other abstract subclasses of Element. I'm just trying to understand what is different here.

@jdemeyer
Copy link
Author

comment:6

It's a valid question... we do have to add those declarations somewhere and it seemed silly to do it for every finite field element class separately. This abstract base class looked like the correct level of abstraction: it's still rather concrete (vs. Element or RingElement for example). On the other hand: for polynomials, we do add it to every concrete class.

@tscrim
Copy link
Collaborator

tscrim commented Oct 29, 2017

comment:7

Ah, yea, right. LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Oct 29, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Oct 29, 2017

comment:8

Also to anyone else looking at this change, having a doctest here is functionally pointless as these are here mainly as a technical detail. So that it why I positively reviewed this without explicit doctests for those methods.

@vbraun
Copy link
Member

vbraun commented Nov 1, 2017

Changed branch from u/jdemeyer/fix_cython_warnings_in_finite_rings to 9dbd211

@jdemeyer
Copy link
Author

Changed commit from 9dbd211 to none

@jdemeyer
Copy link
Author

comment:10

I plan to move this way down to element.pyx in #24607.

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