Fix the remaining cpdef declaration issues concerning vtables

roed314 commented 7 years ago

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:

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

CC: @jdemeyer

Component: cython

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

roed314 commented 7 years ago

comment:1

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

jdemeyer commented 7 years ago

comment:2

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, ...)

jdemeyer commented 7 years ago

Branch: u/jdemeyer/fix_the_remaining_cpdef_declaration_issues

jdemeyer commented 7 years ago

New commits:

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

jdemeyer commented 7 years ago

Commit: 7fdcbe7

jdemeyer commented 7 years ago

comment:5

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.

jdemeyer commented 7 years ago

comment:6

Comments in more detail:

sage/algebras/quatalg: instead, add abstract methods _add_ and _mul_ to the base class.
sage/modular/pollack_stevens: unrelated change to cpdef 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 class GenericBackend
sage/rings/finite_rings: instead, add abstract methods _add_ and _mul_ to the base class.
src/sage/rings/number_field: already in the base class NumberFieldElement (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.

jdemeyer commented 7 years ago

comment:7

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.

roed314 commented 7 years ago

comment:8

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.

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -1 +1,17 @@
-In #23227 we added `cpdef` declarations to address https://github.com/cython/cython/issues/1732.  A few were not completely straightforward and left for a followup ticket.
+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`
+
+2. `sage/modular/pollack_stevens`: see #23603
+
+3. `src/sage/modules`
+
+4. `sage/numerical/backends`
+
+5. `sage/rings/finite_rings`
+
+6. `src/sage/rings/number_field`: see #23603
+
+7. `sage/rings/padics`
+
+8. `src/sage/rings/polynomial` and power series

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -4,14 +4,14 @@

 2. `sage/modular/pollack_stevens`: see #23603

-3. `src/sage/modules`
+3. `sage/modules`

 4. `sage/numerical/backends`

 5. `sage/rings/finite_rings`

-6. `src/sage/rings/number_field`: see #23603
+6. `sage/rings/number_field`: see #23603

 7. `sage/rings/padics`

-8. `src/sage/rings/polynomial` and power series
+8. `sage/rings/polynomial` and power series

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,6 @@
 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`
+1. `sage/algebras/quatalg/quaternion_algebra_element.pxd`

 2. `sage/modular/pollack_stevens`: see #23603

@@ -14,4 +14,6 @@

 7. `sage/rings/padics`

-8. `sage/rings/polynomial` and power series
+8. `sage/rings/polynomial/laurent_polynomial.pxd
+
+9. `sage/rings/power_series_*`

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -14,6 +14,6 @@

 7. `sage/rings/padics`

-8. `sage/rings/polynomial/laurent_polynomial.pxd
+8. `sage/rings/polynomial/laurent_polynomial.pxd`

 9. `sage/rings/power_series_*`

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -14,6 +14,6 @@

 7. `sage/rings/padics`

-8. `sage/rings/polynomial/laurent_polynomial.pxd`
+8. `sage/rings/polynomial/laurent_polynomial.pxd`: #23606

 9. `sage/rings/power_series_*`

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -6,7 +6,7 @@

 3. `sage/modules`

-4. `sage/numerical/backends`
+4. `sage/numerical/backends`: see #20324

 5. `sage/rings/finite_rings`

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -2,15 +2,15 @@

 1. `sage/algebras/quatalg/quaternion_algebra_element.pxd`

-2. `sage/modular/pollack_stevens`: see #23603
+2. `sage/modular/pollack_stevens`: #23603

 3. `sage/modules`

-4. `sage/numerical/backends`: see #20324
+4. `sage/numerical/backends`: #20324

 5. `sage/rings/finite_rings`

-6. `sage/rings/number_field`: see #23603
+6. `sage/rings/number_field`: #23603

 7. `sage/rings/padics`

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -8,7 +8,7 @@

 4. `sage/numerical/backends`: #20324

-5. `sage/rings/finite_rings`
+5. `sage/rings/finite_rings`: #24116

 6. `sage/rings/number_field`: #23603

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -1,14 +1,14 @@
 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`
+1. `sage/algebras/quatalg/quaternion_algebra_element.pxd`: #24607

 2. `sage/modular/pollack_stevens`: #23603

-3. `sage/modules`
+3. `sage/modules`: #24607

 4. `sage/numerical/backends`: #20324

-5. `sage/rings/finite_rings`: #24116
+5. `sage/rings/finite_rings`: #24116 and #24607

 6. `sage/rings/number_field`: #23603

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -16,4 +16,4 @@

 8. `sage/rings/polynomial/laurent_polynomial.pxd`: #23606

-9. `sage/rings/power_series_*`
+9. `sage/rings/power_series_*`: #24607, #24608

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -16,4 +16,4 @@

 8. `sage/rings/polynomial/laurent_polynomial.pxd`: #23606

-9. `sage/rings/power_series_*`: #24607, #24608
+9. `sage/rings/power_series_*`: #24607 and #24608

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -12,7 +12,7 @@

 6. `sage/rings/number_field`: #23603

-7. `sage/rings/padics`
+7. `sage/rings/padics`: #24609

 8. `sage/rings/polynomial/laurent_polynomial.pxd`: #23606

jdemeyer commented 6 years ago

comment:20

Every issue has a ticket now, so let's close this.

jdemeyer commented 6 years ago

Changed branch from u/jdemeyer/fix_the_remaining_cpdef_declaration_issues to none

jdemeyer commented 6 years ago

Changed commit from 7fdcbe7 to none

jdemeyer commented 5 years ago

Description changed:

--- 
+++ 
@@ -12,7 +12,7 @@

 6. `sage/rings/number_field`: #23603

-7. `sage/rings/padics`: #24609
+7. `sage/rings/padics`: #27417, #24609

 8. `sage/rings/polynomial/laurent_polynomial.pxd`: #23606

sagemath / sage

Fix the remaining cpdef declaration issues concerning vtables #23600