-
Notifications
You must be signed in to change notification settings - Fork 5
update ldexp types
#73
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
Conversation
8395aab to
0b9eeee
Compare
|
Since this branch is currently being used to debug numpy test failures, I wonder if it'd be worth making this change in this PR: diff --git a/CMakeLists.txt b/CMakeLists.txt
index af4e480..c421df7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -45,6 +45,7 @@ if(WIN32)
string(CONCAT PRECISION_FLAGS
"/fp:fast=2 "
"/Qimf-precision=high "
+ "/Qprec-sqrt "
"/Qprotect-parens "
)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /Ox ${WARNING_FLAGS} ${SDL_FLAGS} ${PRECISION_FLAGS}")
@@ -81,6 +82,7 @@ elseif(UNIX)
"${SDL_FLAGS}"
)
string(CONCAT PRECISION_FLAGS
+ "-prec-sqrt "
"-fprotect-parens "
"-fimf-precision=high "
"-fp-model fast=2 "
Using |
AndresGuzman-Ballen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within the context of fixing the ldexp issue, I think this PR is good!
|
@AndresGuzman-Ballen What's interesting is that it is apparently no longer going to be supported—but if this means test failures, do we know what alternative they recommend? Note that following their recommendation to use |
@ndgrigorian |
|
@vtavana correct! @ndgrigorian sorry for the confusion. |
antonwolfy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the changelog needs to be updated with that change
ndgrigorian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
By the way, will we need to increment the patch version number for mkl_umath after this is merged? |
Do you mean the package version? I figured we would cut a new version around the time of the release, but if numpy builds need it, I can go ahead and do it whenever. |
|
@ndgrigorian good point! I'll keep an eye out for new versions around release time then :) thanks! |
With stock NumPy 2.2.5 on Windows we have
While for Stock NumPy 2.2.5 on Linux we have
The difference is that we have
'q'on Windows instead of'l'on Linux.With Intel/Stock NumPy 1.26.4, on both Windows and Linux we have
'l'.mkl_umathcurrently has functions for type 'fl->f' and 'dl->d' which have no equivalent in Stock NumPy 2.2.5 on Windows.This PR resolves this issue. The changes I made is based on what is done in numpy-2.x.x.
With this branch