aggregations.pyx: use std:: for signbit() and sqrt().

[he32]

This reintroduces how this was done in pandas 1.3.5. This should fix issue #51047.

• closes BUG: aggregations.pyx importing signbit() and sqrt() #51047
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

[jbrockmendel]

Can we get this from numpy?

worth up streaming a fix to cython?

[WillAyd]

Thanks a ton for the research. I agree with @jbrockmendel though I think this change should be done upstream in Cython

[he32]

Thanks a ton for the research. I agree with @jbrockmendel though I think this change should be done upstream in Cython

Really? Well, OK. Who takes that up with the Cython maintainers? Myself I barely know python, and much less Cython, so feel kind of lost.

[WillAyd]

Hmm on second thought this might be something we need to handle on our end. What is importing the C++ complex header? Is that done via deque?

[he32]

Hmm on second thought this might be something we need to handle on our end. What is importing the C++ complex header? Is that done via deque?

The generated aggregations.cpp contains

/* Header.proto */
#if !defined(CYTHON_CCOMPLEX)
  #if defined(__cplusplus)
    #define CYTHON_CCOMPLEX 1
  #elif defined(_Complex_I)
    #define CYTHON_CCOMPLEX 1
  #else
    #define CYTHON_CCOMPLEX 0
  #endif
#endif
#if CYTHON_CCOMPLEX
  #ifdef __cplusplus
    #include <complex>
  #else
    #include <complex.h>
  #endif
#endif

but that is long after

#include "Python.h"

in the generated aggregations.cpp file. Python.h includes pyport.h, which in turn includes <math.h>. So, since this is a C++ file, <complex> gets included, long after <math.h> is included.

CYTHON_CCOMPLEX is not defined elsewhere in the agggrergations.cpp file, and therefore, on NetBSD at least, you end up with signbit() and sqrt() only being available via the C++ std:: name space at that point.

This is with Python 3.10 on NetBSD/macppc 10.0_BETA and cython 0.29.33.

[WillAyd]

Awesome - appreciate all the details again. Yea this is a bit tricky - I see why your solution works but (especially since we don't have CI set up for your issue) am wary of how fragile it will be for us over time.

Does importing as an alias from Cython have any effect, i.e. doing from libc.math import signbit as c_signbit? Or do things still get clobbered that way

@scoder may have a better solution

[github-actions]

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

[scoder]

There's a PR for adding libcpp.cmath declarations to Cython 3.0: cython/cython#5262

from libc.math import signbit as c_signbit

This won't change anything in the generated C/C++ code.

Since yo appear to be generating C++ code here, it seems reasonable to use the C+ math functions from their respective namespace. That's generally safer than using something prone to naming collisions from the flat global C namespace.

[mroeschke]

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.

[he32]

I'm sorry, I ran out of energy trying to understand what would be your alternative suggestions. To me it seems "more correct" and less ambigious and less dependent on the particular layout of the C header files to use the std:: versions of these functions when doing so from C++, as is the case here.

[WillAyd]

Sorry lost in the shuffle but from the conversation I think this is ok. If you want to meet in main and get the CI green I would approve

[WillAyd]

Err wait...from the PR linked above it looks like Cython has libcpp.math - can you import from there now?