Opened 4 years ago

Last modified 2 years ago

#23600 closed defect

Fix the remaining cpdef declaration issues — at Version 17

Reported by: roed Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: cython Keywords:
Cc: jdemeyer Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: u/jdemeyer/fix_the_remaining_cpdef_declaration_issues (Commits, GitHub, GitLab) Commit: 7fdcbe7a64ce7bc55d715e76f2d2cf7d4d342e74
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

In #23227 we added cpdef declarations to address most of https://github.com/cython/cython/issues/1732. A few were not completely straightforward and will be dealt with in follow-up tickets:

  1. sage/algebras/quatalg/quaternion_algebra_element.pxd: #24607
  1. sage/modular/pollack_stevens: #23603
  1. sage/modules: #24607
  1. sage/numerical/backends: #20324
  1. sage/rings/finite_rings: #24116 and #24607
  1. sage/rings/number_field: #23603
  1. sage/rings/padics
  1. sage/rings/polynomial/laurent_polynomial.pxd: #23606
  1. sage/rings/power_series_*

Change History (17)

comment:1 Changed 4 years ago by roed

Jeroen, can you add the code here that you removed from the other branch?

comment:2 Changed 4 years ago by jdemeyer

I would rather prefer to fix this on a case-by-case basis (for example, one ticket for p-adics, one ticket for quaternion algebras, ...)

comment:3 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/fix_the_remaining_cpdef_declaration_issues

comment:4 Changed 4 years ago by jdemeyer

  • Commit set to 7fdcbe7a64ce7bc55d715e76f2d2cf7d4d342e74

New commits:

7fdcbe7Add cpdef _add_(self, other) and cpdef _mul_(self, other) all over

comment:5 Changed 4 years ago by jdemeyer

To be clear: I am not saying that I'm against all the changes in this remaining branch, only that it's not immediately clear that it's the right thing to do.

comment:6 Changed 4 years ago by jdemeyer

Comments in more detail:

  1. sage/algebras/quatalg: instead, add abstract methods _add_ and _mul_ to the base class.
  1. sage/modular/pollack_stevens: unrelated change to cpdef normalize
  1. src/sage/modules: instead, add abstract methods _add_ and _mul_ to the base class.
  1. sage/numerical/backends: add_variable is already declared in the base class GenericBackend
  1. sage/rings/finite_rings: instead, add abstract methods _add_ and _mul_ to the base class.
  1. src/sage/rings/number_field: already in the base class NumberFieldElement (but not declared there)
  1. sage/rings/padics: lots of changes, not looked in detail yet.
  1. src/sage/rings/polynomial and power series: instead, add abstract methods to the base class.
Last edited 4 years ago by jdemeyer (previous) (diff)

comment:7 Changed 4 years ago by jdemeyer

I can try to fix some of these, but we should coordinate to avoid double work. Items 2, 4 and 6 on the list above seem easiest, so I will have a quick look now.

EDIT: 2 and 4 are different cpdef declarations issues, they are because of a changed signature. I sent an e-mail to cython-users asking whether the warning is still correct in this case.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:8 Changed 4 years ago by roed

Okay. I'm traveling at the moment, so I'm not going to work on this immediately. I'll make a comment here before doing anything.

comment:9 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:12 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:13 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:14 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:15 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:16 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:17 Changed 3 years ago by jdemeyer

  • Description modified (diff)
Note: See TracTickets for help on using tickets.