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

Implement <numbers> Math Constants #261

Merged
merged 14 commits into from
Nov 12, 2019
Merged

Conversation

SuperWig
Copy link
Contributor

@SuperWig SuperWig commented Nov 5, 2019

Description

Resolves #29. As partially discussed there I just pulled the values from Wolfram Alpha to the same significant figures as M_PI and co.

Not sure if it was wanted but I provided both a concepts and non-concepts verion. Concepts version depends on #251

I created this test program as requested and it all appears to check out (P.S. I have no idea what I'm doing so I could be wrong, also praise be to fmt)

One thing to note is that static_assert(1 / sqrt2 == M_SQRT1_2); fails with

0.70710678118654746 == 
0.70710678118654757

So unlike the paper suggests you probably can't redefine that one in terms of std::numbers
Sidenote: why does <cmath> not work for the constants? Are the docs incorrect there?

And if I understand #140 correctly I should add _STD for floating_point, enable_if_t, and is_floating_point?

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository and
    the C++ Working Draft as a reference (and any other cited standards).
    If they were derived from a project that's already listed in NOTICE.txt,
    that's fine, but please mention it. If they were derived from any other
    project (including Boost and libc++, which are not yet listed in
    NOTICE.txt), you must mention it here, so we can determine whether the
    license is compatible and what else needs to be done.

@SuperWig SuperWig requested a review from a team as a code owner November 5, 2019 16:16
@contre
Copy link

contre commented Nov 5, 2019

I am no expert on the rules for implicit type conversions but for the 1 / sqrt2 assert, could that be an issue? Does the fact you’ve got integer 1, an M_ constant which I assume is double, and whatever sqrt resolves cause any concern?

@SuperWig
Copy link
Contributor Author

SuperWig commented Nov 6, 2019

Changing it to 1. / sqrt2 doesn't change that.


Is #251 still an issue given that's how the standard defines it https://eel.is/c++draft/concepts.arithmetic?

@StephanTLavavej
Copy link
Member

As partially discussed there I just pulled the values from Wolfram Alpha to the same significant figures as M_PI and co.

I'm torn between wanting hexfloats and wanting shortest round-trip doubles. Probably the latter.

Thanks for the test program; it increases my confidence in these changes.

One thing to note is that static_assert(1 / sqrt2 == M_SQRT1_2); fails

That doesn't seem surprising to me; the sqrt2 constant has been quantized to binary, so inverting it doesn't necessarily produce the same result as inverting the infinitely precise real number and then quantizing that. If anything, it's more surprising when this results in equality.

Sidenote: why does <cmath> not work for the constants? Are the docs incorrect there?

Those constants are non-Standard, I believe.

And if I understand #140 correctly I should add _STD for floating_point, enable_if_t, and is_floating_point?

Good question; the answer is no. There are three major kinds of identifiers: non-_Ugly functions like equal, _Ugly functions like _Integer_to_chars, and non-functions. For non-_Ugly functions, we must _STD qualify (expands to ::std::) to avoid unintentional Argument-Dependent Lookup. There are only a few cases where ADL is desired (e.g. swap). For _Ugly functions, which users can't mention (as such identifiers are reserved), we currently don't qualify them, since ADL isn't usually a concern if users can't create conflicts. #140 tracks whether we should change that convention. Finally, for non-functions (like classes, alias templates, variable templates, etc.), ADL isn't a concern (that's only for functions). The rules of name lookup ensure that "our" identifiers (within std) will be found before any other identifiers outside. Therefore, we don't qualify non-function identifiers, except in special situations (e.g. reverse_iterator is sometimes a typedef defined by a class; qualifying the name makes it clear that we want the class template in std).

given that's how the standard defines it https://eel.is/c++draft/concepts.arithmetic?

That's not an issue; please go ahead and define the floating_point concept in this PR as described by the Working Paper. Our concern is with that user, who has a history of blatantly disregarding licenses, not with the concept.

stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Show resolved Hide resolved
stl/inc/numbers Show resolved Hide resolved
stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

/azp run

@StephanTLavavej
Copy link
Member

Today the Azure Pipelines bot isn't paying attention to my comments for unknown reasons (it's happening in another PR too). We'll investigate.

@SuperWig
Copy link
Contributor Author

SuperWig commented Nov 7, 2019

Sidenote: why does not work for the constants? Are the docs incorrect there?
Those constants are non-Standard, I believe.

Your docs say

#define _USE_MATH_DEFINES // for C++
#include <cmath>

Which doesn't seem to work. Though I guess it should now point to the docs for this header now (or have them on that page), so guess this is moot.


I totally forgot about the specialisation aspect, so sorry about that. Not sure about the static_assert message I put for the primary template.

Also should the non-concepts version be simplified with a macro (or TMP which I don't have the skills for)?

@BillyONeal
Copy link
Member

Your docs say

#define _USE_MATH_DEFINES // for C++
#include
Which doesn't seem to work

Regardless of what the docs say, _USE_MATH_DEFINES is a user setting. We have to assume the user has not set it.

stl/inc/numbers Outdated Show resolved Hide resolved
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

This is looking good; I think after one more revision we'll be able to port this to MSVC-internal git and enable some basic tests. Thanks!

stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Show resolved Hide resolved
stl/inc/numbers Show resolved Hide resolved
stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Outdated Show resolved Hide resolved
stl/inc/numbers Show resolved Hide resolved
stl/inc/numbers Show resolved Hide resolved
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks! I'm going to port this to our MSVC-internal repo now and begin adding setup/test changes.

stl/inc/numbers Show resolved Hide resolved
stl/inc/numbers Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Nov 12, 2019

One thing to note is that static_assert(1 / sqrt2 == M_SQRT1_2); fails

I verified that this is by design, caused by the quantized nature of binary floating-point. Here is a full analysis:

#define _USE_MATH_DEFINES
#include <cmath>
#include <numbers>
#include <cstdio>
using namespace std;
using namespace std::numbers;

int main() {
    printf("      sqrt2: %a %.1000g\n\n", sqrt2, sqrt2);
    printf("1.0 / sqrt2: %a %.1000g\n", 1.0 / sqrt2, 1.0 / sqrt2);
    puts("Exact inverse of quantized sqrt2: 0.7071067811865474760643777948402871575373580514923651957447...");
    puts("  0x1.6a09e667f3bcc8p-1 midpoint: 0.707106781186547517226159698111587204039096832275390625");
    puts("  Exact inverse of exact sqrt(2): 0.7071067811865475244008443621048490392848359376884740365883...");
    printf("  M_SQRT1_2: %a %.1000g\n", M_SQRT1_2, M_SQRT1_2);
    printf("sqrt2 / 2.0: %a %.1000g\n", sqrt2 / 2.0, sqrt2 / 2.0);
}
C:\Temp>cl /EHsc /nologo /W4 /std:c++latest sqrt.cpp && sqrt
sqrt.cpp
      sqrt2: 0x1.6a09e667f3bcdp+0 1.4142135623730951454746218587388284504413604736328125

1.0 / sqrt2: 0x1.6a09e667f3bccp-1 0.707106781186547461715008466853760182857513427734375
Exact inverse of quantized sqrt2: 0.7071067811865474760643777948402871575373580514923651957447...
  0x1.6a09e667f3bcc8p-1 midpoint: 0.707106781186547517226159698111587204039096832275390625
  Exact inverse of exact sqrt(2): 0.7071067811865475244008443621048490392848359376884740365883...
  M_SQRT1_2: 0x1.6a09e667f3bcdp-1 0.70710678118654757273731092936941422522068023681640625
sqrt2 / 2.0: 0x1.6a09e667f3bcdp-1 0.70710678118654757273731092936941422522068023681640625

Note the difference between 1.0 / sqrt2 and sqrt2 / 2.0. Mathematically, 1 / sqrt(2) and sqrt(2) / 2 are equal, but because sqrt2 has been rounded to double, these expressions don't behave identically. For 1.0 / sqrt2, my understanding is that division is one of the few operations that IEEE guarantees is performed exactly and then rounded. So the exact inverse is (conceptually) obtained and then rounded, producing the observed result. For sqrt2 / 2.0, that operation is also conceptually performed exactly and then rounded - but because it's division by two, the exponent can just be adjusted, and the mantissa is not changed by rounding. This is why M_SQRT1_2 and sqrt2 / 2.0 have the same mantissa as sqrt2, with a different exponent.

@StephanTLavavej StephanTLavavej merged commit eba6a71 into microsoft:master Nov 12, 2019
@StephanTLavavej
Copy link
Member

Thanks again, and congratulations for your second C++20 feature!

@SuperWig
Copy link
Contributor Author

Thanks! Though I don't think moving something you already implemented counts 😛

@SuperWig SuperWig deleted the numbers branch November 12, 2019 07:21
@miscco
Copy link
Contributor

miscco commented Nov 12, 2019

int main() {
printf(" sqrt2: %a %.1000g\n\n", sqrt2, sqrt2);
printf("1.0 / sqrt2: %a %.1000g\n", 1.0 / sqrt2, 1.0 / sqrt2);
puts("Exact inverse of quantized sqrt2: 0.7071067811865474760643777948402871575373580514923651957447...");
puts(" 0x1.6a09e667f3bcc8p-1 midpoint: 0.707106781186547517226159698111587204039096832275390625");
puts(" Exact inverse of exact sqrt(2): 0.7071067811865475244008443621048490392848359376884740365883...");
printf(" M_SQRT1_2: %a %.1000g\n", M_SQRT1_2, M_SQRT1_2);
printf("sqrt2 / 2.0: %a %.1000g\n", sqrt2 / 2.0, sqrt2 / 2.0);
}

@StephanTLavavej That you used printf instead of to_chars feels wrong, Nice analysis though

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

Successfully merging this pull request may close these issues.

P0631R8 <numbers> Math Constants
6 participants