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

BUG: Fix M_PI #12160

Merged
merged 3 commits into from May 19, 2020
Merged

BUG: Fix M_PI #12160

merged 3 commits into from May 19, 2020

Conversation

larsoner
Copy link
Member

Might fix https://ci.appveyor.com/project/scipy/scipy/builds/32959985 and related failures:

  scipy\stats\_stats.c(19781): error C2065: 'M_PI': undeclared identifier

Seems like this must be an underlying Cython issue (?), but maybe this will allow us to work around it.

@WarrenWeckesser
Copy link
Member

WarrenWeckesser commented May 18, 2020

The definition of M_PI in math.h is not standard. Perhaps what we really need is to define _USE_MATH_DEFINES when building with a Microsoft compiler: https://docs.microsoft.com/en-us/cpp/c-runtime-library/math-constants?view=vs-2019

@larsoner
Copy link
Member Author

It seems like something that Cython should take care of when you use libc.math, no?

In any case, it seems to have succeeded building with this:

https://ci.appveyor.com/project/scipy/scipy/builds/32961266

Might be worth merging this as-is to get things back to green, then if someone has a more principled solution, that can undo the change here (if desired, it seems fairly harmless).

@larsoner
Copy link
Member Author

The build passed but it's not functional, trying to fix the shim...

@larsoner
Copy link
Member Author

Actually it looks like elsewhere in the codebase we solve this in various different ways. It seems like the most expedient here would be just to use PI from numpy.math, which is already cimported. Let's see if that one passes.

@larsoner
Copy link
Member Author

Seems to have fixed AppVeyor https://ci.appveyor.com/project/scipy/scipy/builds/32962123

Copy link
Member

@WarrenWeckesser WarrenWeckesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even simpler than the first version!

@WarrenWeckesser
Copy link
Member

Any idea why there is no Travis-CI run?

@larsoner
Copy link
Member Author

Could be just some slow status update issue (or maybe someone restarted it?), but it's there now and green

@WarrenWeckesser
Copy link
Member

Yes, I guess it is just very slow getting started. It wasn't even in the list when I made my comment

@WarrenWeckesser WarrenWeckesser merged commit 8ba0b8e into scipy:master May 19, 2020
@WarrenWeckesser
Copy link
Member

Merged. Thanks, @larsoner.

@WarrenWeckesser WarrenWeckesser added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.stats labels May 19, 2020
@larsoner larsoner deleted the m_pi branch May 19, 2020 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants