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 np.clip bug NaN handling for Visual Studio 2015 #7678

Merged

Conversation

lesteve
Copy link
Contributor

@lesteve lesteve commented May 26, 2016

Fix #7601.

The work-around is to disable the loop vectorization for Visual studio 2015. Let me know if there is a better way to achieve the same results.

I added a test that highlights the issue (only python 3.5).

In a separate branch I made sure that the test was failing without the loop vectorization disabled: AppVeyor build.

and that disabling the loop vectorization fixed the problem: AppVeyor build.

by disabling loop vectorization and add test.
@juliantaylor
Copy link
Contributor

hm, the bug says this is supposed to be fixed in vs2015, is 1900 the 2015 version?

@lesteve
Copy link
Contributor Author

lesteve commented May 26, 2016

hm, the bug says this is supposed to be fixed in vs2015, is 1900 the 2015 version?

Short answer is yes.

Long answer: from https://msdn.microsoft.com/en-us/library/b0084kay.aspx about _MSC_VER:

For example, if the version number of the Visual C++ compiler is 17.00.51106.1, the _MSC_VER macro evaluates to 1700. Type cl /? at the command line to view the compiler's version number.

cl /? in a Developer Command Prompt for VS2015 gives:

Microsoft (R) C/C++ Optimizing Compiler Version 19.00.23918 for x86

@juliantaylor
Copy link
Contributor

but this disables vectorization for 2015 then which is supposed to be fixed.

@lesteve
Copy link
Contributor Author

lesteve commented May 26, 2016

but this disables vectorization for 2015 then which is supposed to be fixed.

Hmm, where do you get the impression that vectorization is supposed to be fixed in VS 2015?

According to the ticket I opened at connect.microsoft.com there, one person from Microsoft acknowledged that there was a bug in the latest version of the compiler:

I’m sorry, after sending the previous message I saw that I didn’t test your repro accurately. I can repro the bug even in the latest version, so I am re-opening this Connect and will continue to investigate.

@juliantaylor
Copy link
Contributor

Thanks for the excellent bug report and repro. Fortunately, this bug was found and fixed and should be available from VS2015 and later versions; I tested your repro to confirm. So upgrading to the latest version of the compiler should address this issue.

@lesteve
Copy link
Contributor Author

lesteve commented May 26, 2016

The message you copied and pasted was posted 40 minutes before the one I copied and pasted. Confusingly the latest message is at the top in their bug tracker ...

@juliantaylor
Copy link
Contributor

oh, that is a bit confusing.

Looks good then, thanks for investigating and reporting the issue.

@juliantaylor juliantaylor merged commit a075c36 into numpy:master May 26, 2016
@lesteve lesteve deleted the fix-python35-windows-clip-with-nan-bug branch May 26, 2016 12:44
@juliantaylor
Copy link
Contributor

@charris probably worth a backport to 11.1, low risk and having it in a stable release soon helps with appveyor tests of other projects

@juliantaylor juliantaylor added the 08 - Backport Used to tag backport PRs label May 26, 2016
@lesteve
Copy link
Contributor Author

lesteve commented May 26, 2016

@charris probably worth a backport to 11.1, low risk and having it in a stable release soon helps with appveyor tests of other projects

That would be great indeed!

@@ -3763,6 +3763,12 @@ static void
}
}
else {
// Visual Studio 2015 loop vectorizer handles NaN in an unexpected manner, see:
Copy link
Member

Choose a reason for hiding this comment

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

Need to fix the C++ comments. I'll do that.

@charris
Copy link
Member

charris commented May 26, 2016

@charris
Copy link
Member

charris commented May 26, 2016

Does the pragma only apply to the following for loop, or are all loops after affected?

EDIT: nvm, looked it up and it is following loop specific.

@Flamefire
Copy link
Contributor

I have encountered a similar issue on POWER9 with GCC 9.3.0 and numpy 1.16.6 (Python 2) where GCC misoptimizes #7601 (comment) so it yields the upper bound on NaN at -O1

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

Successfully merging this pull request may close these issues.

[Windows Python 3.5 only] np.clip replace nans with lower bound
4 participants