Opened 2 years ago

Closed 2 years ago

Last modified 21 months ago

#24116 closed enhancement (fixed)

Fix Cython warnings in finite_rings

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.1
Component: cython Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 9dbd211 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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

This is part of #23600

Change History (10)

comment:1 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/fix_cython_warnings_in_finite_rings

comment:3 Changed 2 years ago by jdemeyer

  • Commit set to 9dbd211a084e00b747dcd01f2c42bfd4d203ce7e
  • Status changed from new to needs_review

New commits:

9dbd211Add abstract _add_ and _mul_ methods to FiniteRingElement

comment:4 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 2 years ago by tscrim

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.

comment:6 Changed 2 years ago by jdemeyer

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.

comment:7 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Ah, yea, right. LGTM.

comment:8 Changed 2 years ago by tscrim

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.

comment:9 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/fix_cython_warnings_in_finite_rings to 9dbd211a084e00b747dcd01f2c42bfd4d203ce7e
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:10 Changed 21 months ago by jdemeyer

  • Commit 9dbd211a084e00b747dcd01f2c42bfd4d203ce7e deleted

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

Note: See TracTickets for help on using tickets.