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

Use C++17 function ellint_1 in adams.cpp #4388

Closed
wants to merge 1 commit into from
Closed

Conversation

cffk
Copy link
Contributor

@cffk cffk commented Jan 24, 2025

Replace approximate series for F(x, 1/sqrt(2)) by a call to ellint_1.

The approximate series was only accurate to "better than 1e-7", which meant that the errors in the projection were typically about 1 m. The adoption of ellint_1 (with its considerably better accuracy) then leads to a torrent of errors in the tests. These have been fixed by changing the tolerances from 1 mm to 1 m.

The values in the tests should be replaced by more accurate ones, but I'm reluctant merely to update the values with the ones the code now produces given that the code depends on a parameter TOL = 1e-9. Also, I don't understand why K(1/sqrt(2)) = std::comp_ellint_1(RSQRT2) = 1.8540746773013719 is sometimes given to only 5 decimal places as 1.85407.

  • Tests updated
  • Added clear title that can be used to generate release notes
  • Fully documented in commit message

Replace approximate series for F(x, 1/sqrt(2)) by a call to ellint_1.

The approximate series was only accurate to "better than 1e-7", which
meant that the errors in the projection were typically about 1 m.  The
adoption of ellint_1 (with its considerably better accuracy) then leads
to a torrent of errors in the tests.  These have been fixed by changing
the tolerances from 1 mm to 1 m.

The values in the tests should be replaced by more accurate ones, but
I'm reluctant merely to update the values with the ones the code now
produces given that the code depends on a parameter TOL = 1e-9.  Also, I
don't understand why K(1/sqrt(2)) = std::comp_ellint_1(RSQRT2) =
1.8540746773013719 is sometimes given to only 5 decimal places as
1.85407.
@rouault
Copy link
Member

rouault commented Jan 25, 2025

it seems clang 18.1.8 both on MacOS with Conda and OSSFuzz Linux lacks support for std::ellint_1() which feels weird (the failure on AppVeyor is likely due to it using MSVC17. we should probably retire that configuration)

@cffk
Copy link
Contributor Author

cffk commented Jan 25, 2025

This is non-essential PR. I was just seeing how C++17 provides. I close this for now and we can revisit this in a year or so.

@cffk cffk closed this Jan 25, 2025
@rouault
Copy link
Member

rouault commented Jan 25, 2025

I was just seeing how C++17 provides

that's a bit strange as my reading of https://en.cppreference.com/w/cpp/numeric/special_functions/ellint_1 is that it should be a mandatory feature of a C++17 compliant environment (there are some parts sometimes of the C++ standards that are optional). Wondering if there aren't mixup of compilers and header of standard library from older compiler, or something like that

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.

2 participants