Skip to content

replace deprecated numpy.math use with libc.math#548

Open
chadawagner wants to merge 1 commit into
CPJKU:mainfrom
chadawagner:cw/fix-bad-cython
Open

replace deprecated numpy.math use with libc.math#548
chadawagner wants to merge 1 commit into
CPJKU:mainfrom
chadawagner:cw/fix-bad-cython

Conversation

@chadawagner

@chadawagner chadawagner commented May 12, 2025

Copy link
Copy Markdown

Changes proposed in this pull request

Use libc.math to import INFINITY rather than deprecated numpy.math

https://github.com/cython/cython/blob/e2e63cb22b2fcc4f5a3780707b0b24fb6be0ffe1/Cython/Includes/numpy/math.pxd#L21-L23

This pull request fixes #547.

Note: even though numpy.math was only deprecated and should still work, builds now fail due to numpy missing from Cython 3.1 wheels cython/cython#6859. Presumably that will be corrected, but we should update to use libc.math going forward, and should consider placing an upper bound on dependencies to be resilient to similar issues in the future.

@zby

zby commented May 14, 2025

Copy link
Copy Markdown

Hi @chadawagner,

Thanks so much for this PR and the fix in commit 868e004eb3e36f4359d11458e5849f9d7e52b644!

I was experiencing build failures with madmom due to the numpy.math.INFINITY issue with recent NumPy versions (specifically when building in a clean CI environment). Pointing my project's dependency to your commit in chadawagner/madmom@868e004eb3e36f4359d11458e5849f9d7e52b644 has successfully resolved the critical build errors, and my project's tests are now passing.

I did notice that during the compilation of the Cython extensions with this commit, a couple of warnings are still present:

  1. A recurring warning across multiple compiled C files:

    .../numpy/_core/include/numpy/npy_1_7_deprecated_api.h:17:2: warning: #warning "Using deprecated NumPy API, disable it with " "#define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" [-Wcpp]
    
  2. And a specific warning in madmom/features/beats_crf.c:

    madmom/features/beats_crf.c:21077:86: warning: ‘__pyx_v_next_state’ may be used uninitialized [-Wmaybe-uninitialized]
    ...
    madmom/features/beats_crf.c:20552:7: note: ‘__pyx_v_next_state’ was declared here
    

While these warnings don't seem to break functionality for my use case, I wanted to share this feedback in case it's helpful for further refinements or for when this PR is considered for merging into CPJKU/madmom.

Thanks again for tackling this!

@hphaavikko hphaavikko mentioned this pull request May 18, 2025
@tingled

tingled commented May 29, 2025

Copy link
Copy Markdown

Thanks for this fix @chadawagner !

@ajayarora1235

Copy link
Copy Markdown

will this be merged anytime soon/ is there a way to install from this PR?

@chadawagner

Copy link
Copy Markdown
Author

will this be merged anytime soon/ is there a way to install from this PR?

@ajayarora1235 the issue was only breaking due to missing files in Cython 3.1, which has been fixed in 3.1.2 afaik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cant import madmom

4 participants