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

Add missing cdef method declarations (for Cython 3) #34257

Closed
mkoeppe opened this issue Jul 31, 2022 · 10 comments
Closed

Add missing cdef method declarations (for Cython 3) #34257

mkoeppe opened this issue Jul 31, 2022 · 10 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Jul 31, 2022

from #29863 comment:15

CC: @tscrim @kwankyu @tobiasdiez

Component: cython

Author: Matthias Koeppe

Branch/Commit: u/mkoeppe/add_missing_cdef_method_declarations__for_cython_3_ @ 9d89bdf

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

@mkoeppe mkoeppe added this to the sage-9.7 milestone Jul 31, 2022
@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 31, 2022

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 31, 2022

New commits:

1f9ee80src/sage/libs/lcalc/lcalc_Lfunction.pxd: Add missing cdef method declarations
034094csrc/sage/matroids/{basis,linear}_matroid.pxd: Add missing cdef method declarations
9d89bdfsrc/sage/rings/polynomial/polynomial_element.pxd: Add missing cdef method declaration

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 31, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 31, 2022

Commit: 9d89bdf

@tscrim
Copy link
Collaborator

tscrim commented Jul 31, 2022

comment:3

I am very surprised that Cython will require subclasses to redeclare things in the base class with the same signature. This seems quite pointless and likely to have a number of changes in the Sage library. Well, if it is what it is, then we will have to do it, but it is a departure from C++. You think you got every one here?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 31, 2022

comment:4

Replying to @tscrim:

You think you got every one here?

Yes, as tested with #29863

@tscrim
Copy link
Collaborator

tscrim commented Aug 2, 2022

comment:5

Build failure; see patchbot.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 2, 2022

comment:6

Indeed

[sagelib-9.7.beta6] Error compiling Cython file:
[sagelib-9.7.beta6] ------------------------------------------------------------
[sagelib-9.7.beta6] ...
[sagelib-9.7.beta6]             tmpi=Integer(dirichlet_coeff[i])
[sagelib-9.7.beta6]             coeffs[i+1] = mpz_get_si(tmpi.value)
[sagelib-9.7.beta6]         self.thisptr=new_c_Lfunction_I(NAME, what_type,  N, coeffs, Period, q,  w,  A, g, l, n_poles, p, r)
[sagelib-9.7.beta6]         del_ints(coeffs)
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]     cdef inline c_Complex __value(self,c_Complex s,int derivative):
[sagelib-9.7.beta6]         ^
[sagelib-9.7.beta6] ------------------------------------------------------------
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6] sage/libs/lcalc/lcalc_Lfunction.pyx:510:9: Overriding final methods is not allowed
[sagelib-9.7.beta6] 

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 8, 2022

comment:7

Help by Cython experts is needed to find a solution that works with both Cython 0.29.x and Cython 3

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.8, sage-9.9 Jan 7, 2023
@mkoeppe mkoeppe removed this from the sage-10.0 milestone Apr 30, 2023
@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 22, 2023

Closing this; @tornaria describes a proper solution in #29863 (comment)

@mkoeppe mkoeppe closed this as not planned Won't fix, can't repro, duplicate, stale Jul 22, 2023
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

2 participants