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

update boost-math and fix std::ellint_2 #3077

Merged
merged 15 commits into from
Oct 24, 2022
Merged

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Sep 4, 2022

Fixes #3076 / VSO-1600625 / AB#1600625 / DevCom-10133797

I updated boost-math submodule to 1.80, https://github.com/boostorg/math/releases/tag/boost-1.80.0

I copied assert_close from:

void assert_close(const float f, const float g) {
assert(abs(f - g) < 0.01f);
}

Do we need to update something else like these hashes #2598 ?

Am I correct that this change is an affects redist change and it is blocked for now?

@fsb4000 fsb4000 requested a review from a team as a code owner September 4, 2022 09:22
@StephanTLavavej StephanTLavavej added bug Something isn't working affects redist Results in changes to separately compiled bits labels Sep 7, 2022
@StephanTLavavej
Copy link
Member

I believe we should update the hash in cgmanifest.json, yes.

More importantly, we'll need to update the version of Boost.Math used in the MSVC-internal build, otherwise this fix won't "take effect" for the shipping product. Currently it's using a patched copy of Boost 1.66.0 (the patches are all upstreamed now). I believe it should be fairly simple to drop that and switch to standalone Boost.Math via submodule (with the greatest difficulty being MSBuild), except for the question of test coverage. Currently we have one test for Special Math that Casey prepared by deriving from Boost's tests (and it is internal-only because we haven't had time to deal with it; nothing was fundamentally blocking it from moving to GitHub though). That test will fail with the newer version of Boost.Math due to various implementation changes, probably all intentional.

I don't believe we have a tracking issue for this, but IIRC Casey mentioned that an exhaustive test is probably not necessary. All we would need is a test that exercises every function for a few example data points, to verify that nothing is horribly messed up. I believe the only special focus such a test would need is for the extra logic we have to deal with divergence between the Standard and Boost's requirements (this is clearly commented in the source code).

Would you be interested in adding such test coverage? I think we could find a maintainer to make the MSBuild changes and complete the upgrade.

@fsb4000
Copy link
Contributor Author

fsb4000 commented Sep 7, 2022

Would you be interested in adding such test coverage? I think we could find a maintainer to make the MSBuild changes and complete the upgrade.

Yes, I can add couple tests for every special functions.

@fsb4000
Copy link
Contributor Author

fsb4000 commented Sep 8, 2022

I believe the only special focus such a test would need is for the extra logic we have to deal with divergence between the Standard and Boost's requirements (this is clearly commented in the source code).

Yes, I saw that, but I didn't add yet. I will add them tomorrow.

and I will remove void assert_close from LWG3234_math_special_overloads/test.cpp

@fsb4000
Copy link
Contributor Author

fsb4000 commented Sep 11, 2022

ready for a review

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

Made the test changes I wanted to see; thanks for the work you've done!

@StephanTLavavej
Copy link
Member

The new test is failing for x86 only:

Assertion failed: isclose(beta(2.0, 3.0), 0.08333'3333'3333'3333'3333),
file D:\a\_work\1\s\tests\std\tests\P0226R1_math_special_functions\test.cpp, line 104

I haven't investigated, but perhaps we need an extra ULP?

@strega-nil-ms
Copy link
Contributor

strega-nil-ms commented Sep 21, 2022

Yeah, I investigated - it looks like an issue with the non-SSE2 x86 implementation of Lanczos::lanczos_sum_expG_scaled being less accurate than the SSE2 implementation. It results in a 2 ULP difference from the actual answer.

@AreaZR
Copy link
Contributor

AreaZR commented Oct 11, 2022

Any update on this?

@fsb4000
Copy link
Contributor Author

fsb4000 commented Oct 12, 2022

I think it will be merged in VS 2022 17.6 Preview 2: https://github.com/microsoft/STL/wiki/Changelog#vc-redist-lockdown

Until the redist unlocks in 17.6 Preview 2, we'll be running this test internally (1) statically linked with the new boost::math, and (2) dynamically linked with the old boost::math. The old version still has microsoftGH-3076, so we must avoid the regression test when running internally and using the STL DLLs.
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Just a couple of minor issues that I'll push fixes for.

@CaseyCarter
Copy link
Member

We discussed in the maintainer meeting this week that we think it's fine to merge this even with the redist currently locked. The special math functions aren't the most heavily used, so we are willing to tell people that the workaround for GH-3076 is to link statically until 17.6 preview 2 unlocks the redist and we can update the DLLs.

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! Found a few missing tests, I'll validate and push changes.

tests/std/tests/P0226R1_math_special_functions/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0226R1_math_special_functions/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0226R1_math_special_functions/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0226R1_math_special_functions/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0226R1_math_special_functions/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

@CaseyCarter @strega-nil-ms I pushed minor changes after you approved, to remove an unnecessary static and add meowf/meowl coverage.

I also pushed a conflict-free merge with main and verified that there's no clang-format 15 impact.

@StephanTLavavej StephanTLavavej self-assigned this Oct 21, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 7652eb2 into microsoft:main Oct 24, 2022
@StephanTLavavej
Copy link
Member

Thanks for fixing this bug and adding Special Math test coverage, so that we can now use Boost 1.80.0 to build the official STL DLLs! 😻 🎉 🧮

@fsb4000 fsb4000 deleted the fix3076 branch October 25, 2022 06:38
@StephanTLavavej StephanTLavavej mentioned this pull request May 28, 2024
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects redist Results in changes to separately compiled bits bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<cmath>: Invalid output for incomplete elliptic integral of the second kind with k = 1
6 participants