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:  sageduplicate/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: 
Description (last modified by )
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 followup tickets:
sage/algebras/quatalg/quaternion_algebra_element.pxd
: #24607
sage/modular/pollack_stevens
: #23603
sage/modules
: #24607
sage/numerical/backends
: #20324
sage/rings/number_field
: #23603
sage/rings/padics
sage/rings/polynomial/laurent_polynomial.pxd
: #23606
sage/rings/power_series_*
Change History (17)
comment:1 Changed 4 years ago by
comment:2 Changed 4 years ago by
I would rather prefer to fix this on a casebycase basis (for example, one ticket for padics, one ticket for quaternion algebras, ...)
comment:3 Changed 4 years ago by
 Branch set to u/jdemeyer/fix_the_remaining_cpdef_declaration_issues
comment:4 Changed 4 years ago by
 Commit set to 7fdcbe7a64ce7bc55d715e76f2d2cf7d4d342e74
New commits:
7fdcbe7  Add cpdef _add_(self, other) and cpdef _mul_(self, other) all over

comment:5 Changed 4 years ago by
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
Comments in more detail:
sage/algebras/quatalg
: instead, add abstract methods_add_
and_mul_
to the base class.
sage/modular/pollack_stevens
: unrelated change tocpdef normalize
src/sage/modules
: instead, add abstract methods_add_
and_mul_
to the base class.
sage/numerical/backends
:add_variable
is already declared in the base classGenericBackend
sage/rings/finite_rings
: instead, add abstract methods_add_
and_mul_
to the base class.
src/sage/rings/number_field
: already in the base classNumberFieldElement
(but not declared there)
sage/rings/padics
: lots of changes, not looked in detail yet.
src/sage/rings/polynomial
and power series: instead, add abstract methods to the base class.
comment:7 Changed 4 years ago by
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 email to cythonusers asking whether the warning is still correct in this case.
comment:8 Changed 4 years ago by
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
 Description modified (diff)
comment:10 Changed 4 years ago by
 Description modified (diff)
comment:11 Changed 4 years ago by
 Description modified (diff)
comment:12 Changed 4 years ago by
 Description modified (diff)
comment:13 Changed 4 years ago by
 Description modified (diff)
comment:14 Changed 4 years ago by
 Description modified (diff)
comment:15 Changed 4 years ago by
 Description modified (diff)
comment:16 Changed 4 years ago by
 Description modified (diff)
comment:17 Changed 3 years ago by
 Description modified (diff)
Jeroen, can you add the code here that you removed from the other branch?