-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Conversation
FCALL_CONTRACT; | ||
|
||
// The cmath::atan2 method does not produce nan for atan2(+-inf,+-inf) | ||
return (IS_INFINITY(y) && IS_INFINITY(x)) ? (y / x) : atan2(y, x); |
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.
This code here, returning nan
for atan2(+-inf,+-inf)
goes against the recommended behavior in IEEE 754:2008 and what the MSVC compilers does.
A discussion should be brought up as to if this should maintain backwards compatibility or if this should follow the IEEE standard.
Recommended Behavior (IEEE 754:2008 - Page 45):
atan2(±∞, −∞)
is±3π/4
atan2(±∞, +∞)
is±π/4
This also applies to the COMSingle::Atan2
implementation
@mikedn, Even if it is not accepted, it still gets the ball rolling to get these, or similar, changes into CoreCLR and CoreFX (which I find very exciting :D) We have already partially discussed the changes here dotnet/corefx#1151, and the initial thoughts were positive and in favor of these (or similar) changes. The major discussion point is still how to implement the API, but these changes are easily modified to fit whatever decision is made on that point. |
@tannergooding My acceptance comment was specifically targeted as obsoleting |
@mikedn, Ah, sorry. I missed the link in your original comment. It was briefly discussed in the original proposal, but we could not come to a definitive answer. However, It was brought down to essentially three options:
I opted to go with the second as it seemed to be the best route overall (The first option seems the most unlikely, and the third seemed illogical because we could potentially come across the same issue in the future if a new type was added). |
@tannergooding Thanks for doing this work. It's a lot of good stuff.
|
@KrzysztofCwalina Thanks!
This is definitely still a WIP, but most of the hard work (getting the runtime backing for these) can be done (thankfully) before the API decisions of where to place the functions in the Framework are made. |
The PR was just updated with a small set of fixes to the original.
I am currently working on fixing the Linux/MacOS build. |
@josteink as fyi. There may be some eventual collision with the freebsd bring up he's been working on. |
@MattWhilden As far as I can see, nothing of this collides with my PAL-modifications for FreBSD. Without looking further into it than that, I'm OK with it. |
@tannergooding, one more piece of feedback. As you know, we have a portability story for CoreCLR and the .Net "desktop" CLR. If your changes get added to CoreCLR, but not to the desktop CLR, it will create a friction for portable libraries. Adding such significant feature to the desktop CLR will be very difficult (schedule, deployment, and compatibility wise). This is the reason we often prefer out-of-band features. I will think about this issue more and we can discuss during the API review, but I wanted to make sure you are aware of this issue/tradeoff. |
@josteink Do your PAL-modifications include changes to src\pal\inc\pal.h or src\pal\src\cruntime\finite.cpp or src\pal\src\include\palinternal.h? I am currently working to ensure the behavior of the CMATH functions for Linux/MacOS (CLANG) correspond to the behavior for the Windows (MSVC) versions and it will require modifications to those three files. |
@KrzysztofCwalina Thanks for the heads up. Perhaps there is a way to make the FCALL changes to the desktop and portable CLR (which should be similar) and provide the API through a separate library (similar to the System.Numerics.Vectors features)? |
Not at all. My commit is one new line in pal's context.h. That's all.Jostein Kjønigsen I am currently working to ensure the behavior of the CMATH functions for Linux/MacOS (CLANG) correspond to the behavior for the Windows (MSVC) versions and it will require modifications to those three files. —Reply to this email directly or view it on GitHub. |
Sorry for the noise... My branch didn't rebase properly and resetting it somehow closed the PR. |
Updated the PR with some intrinsic support. Initial tests show |
Would it make sense to calculate square root and derivative using Newton-Raphson method? Here is an example API for Also, see this JS implementation: http://jsperf.com/math-sqrt-vs-newton-raphson-method (IE11 perform worse in Math.sqrt and best in calculating with Newton-Raphson algo) |
@jasonwilliams200OK, I wouldn't imagine that any implementation would be as accurate as, or faster than, the hardware provided intrinsic instruction 'sqrtsd' and 'sqrtss' (which is what calls to Math.Sqrt boil down to). Also, in my own benchmarks and the tables for the JS implementation you gave above, Math.Sqrt outperforms the Newton-Raphson method in all browsers. |
@tannergooding I don't think adding these functions to the primitive types results in the best design for several reasons. First, this reduces the maintainability of the code because the types of arguments depend on which function to use. Second, mscorlib.dll is supposed to only contain the functionality that would be required by many applications. Other not-so-essential functionalities should be in other assemblies, loaded only when needed. Of course, mscorlib is already a mess, but we shouldn't make things worse. Most of these math functions will have very limited usage scenarios. If we include them now in mscorlib, the size of the assembly will increase a tiny bit. Then later, we might add more functions and its size will further increase another bit. Eventually, the new functions will have to be added to another assembly. Third, this can easily confuse those developers that have already implemented many of these functions as extension methods to the primitive types. Another issue is the intention of supporting all math functions that are supported in C++. In that case, we might have to make sure that not only the APIs are identical but also the input/output relation is identical. Otherwise, this can introduce very nasty bugs for those who are converting their codebases between the languages. But then we have this problem of different C++ std implementations with many subtle differences. Finally, what would be the long term plan? When more math functions are added to C++, are we going to stay inline? The C++ APIs may not even be designed in the best way anyway. |
@hadibrais. I appreciate the feedback and have been working on simplifying the scope of the first change, which should be about adding single-precision floating-point functions to match the existing double-precision functions. The first change should also bring the existing double-precision functions inline with current compiler output (there were several FCALLs which could be greatly simplified as various compiler bugs were fixed). As for where they are implemented, I'm afraid that the functions will end up split unless the CoreCLR team decides to take a breaking change. As such, I believe it more intuitive to have them re implemented in the primitive types, where (moving forward) the functions can continue growing and changing without worrying about breaking backwards compatibility. The original functions would continue existing in |
All the original changes are in tannergooding/coreclr#math-changes-original. I am currently working on breaking this up into several commits and reducing the scope to better match the feedback given. |
Am currently working on getting the PAL layer done, including the full set of C Math functions. Even if some of the functions aren't provided via managed code, it is still useful to have them functioning and tested for future scenarios. The Trigonometric and Hyperbolic functions should be done at this point, with a broader and more accurate test coverage. The only interesting scenario I came across was that tan(pi / 2) does not result in INFINITY (on all supported platforms). |
Closing. This has been entirely replaced by #5492, which implements the approved API. |
Fixes dotnet/corefx#1151 - New API for Expanded Math Library.
This is still a work-in-progress.
TODO:
Deprecate(Removed due to feedback from the community)System.Math
System.Double
System.Single
System.Single
andSystem.Double
math APIsSystem.Math
and the CLI).sincos
)Current Work
I am currently working on fixing the Linux/MacOS build. This involves ensuring that the CLANG (or non MSVC) implementations of the cmath functions behave identically to the MSVC implementation.
Edits
System.Math