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

I506 cstdfloat complex std #507

Merged
merged 18 commits into from
Aug 31, 2021
Merged

I506 cstdfloat complex std #507

merged 18 commits into from
Aug 31, 2021

Conversation

ckormanyos
Copy link
Member

This PR addresses #506 via creation of corrected code and new test cases intended to test the corrected behavior.
For initial testing purpoese, the YAML code has been reduced and should not yet be merged in this form

@ckormanyos
Copy link
Member Author

creation of corrected code and new test cases

So far so good on this issue. The fix is anticipated to be relatively straightforward.

...But @mborland the actual float128 tests I'd like to exercise on GHA are not (or seem like they're not) running on GHA. I did significantly reduce the YAML script to just a few cases and compilers, but I would have thought the default Ubuntu setup has libquadmath. Do you see anything I missed in YAML?

@jzmaddock
Copy link
Collaborator

I need to check, but I suspect the tests are being run with -std=c++XX rather than -std=gnu++XX. We need GNU extensions on for __float128.

@mborland
Copy link
Member

@ckormanyos The config shows libquadmath is provided by the base Ubuntu install. Is there another dependency for these tests?

@mborland
Copy link
Member

@jzmaddock None of the tests in the Actions YAML file use gnu++XX. Do some need to get added in for additional variety?

@jzmaddock
Copy link
Collaborator

None of the tests in the Actions YAML file use gnu++XX. Do some need to get added in for additional variety?

I was thinking we should probably add a drone CI config to back up GHA, we could make that all gnu++ by way of variety?

@mborland
Copy link
Member

I was thinking we should probably add a drone CI config to back up GHA, we could make that all gnu++ by way of variety?

It would probably be wise to split the load depending on how fast drone returns results. I do appreciate that GHA knocks out the current test battery in under two hours. My run testing architectures on Travis is still pending from last week...

@jzmaddock
Copy link
Collaborator

It would probably be wise to split the load depending on how fast drone returns results. I do appreciate that GHA knocks out the current test battery in under two hours. My run testing architectures on Travis is still pending from last week...

Drone takes a little longer to pull down the images and get started, but the actual runs are very fast, multiprecision is cycling both drone and GHA in an hour or two.

Travis is currently dead to us: the boostorg account has been suspended for using up too many cycles, it might start up again at some point or not. I've also just disabled a bunch of libraries that have moved to drone in Travis, I fear might have accidentally killed your PR in the process :(

@mborland
Copy link
Member

Drone takes a little longer to pull down the images and get started, but the actual runs are very fast, multiprecision is cycling both drone and GHA in an hour or two.

Sounds like it would be worthwhile to use as backup or continue expanding the tests to include gnu standards on.

Travis is currently dead to us: the boostorg account has been suspended for using up too many cycles, it might start up again at some point or not. I've also just disabled a bunch of libraries that have moved to drone in Travis, I fear might have accidentally killed your PR in the process :(

I can just turn that PR into a remove travis PR then; at that rate it wouldn't offer any useful results anyway. One of the Kernel devs has this repo for testing the kernel on qemu. It may be worthwhile forking it and making it work for testing boost. There does not seem to be any particularly good CI options for testing non x86_64 architectures.

@ckormanyos
Copy link
Member Author

add a drone CI config to back up GHA, we could make that all gnu++ by way of variety?

Yes. Agree with the entire dialog. I will comply in these and the Multiprecision PR(s). At first keep all compilers active, and make a judicious selection at a later point when the load-splitting becomes more clear how to do it...

@jzmaddock
Copy link
Collaborator

Sounds like it would be worthwhile to use as backup or continue expanding the tests to include gnu standards on.

Let me see what I can rustle up.

@mborland
Copy link
Member

@jzmaddock The drone docs shows support for at least Linux and BSD on ARM. I can't imagine we need to do much i386 testing.

@jzmaddock
Copy link
Collaborator

The drone docs shows support for at least Linux and BSD on ARM. I can't imagine we need to do much i386 testing.

Drone is open source and entirely self-hosted, the CppAllience who are providing the service only has x64 clients set up. But yes Drone itself can handle almost any platform and architecture if there's someone to provide the client machines.

@ckormanyos
Copy link
Member Author

for testing the kernel on qemu. It may be worthwhile forking it and making it work for testing boost. There does not seem to be any particularly good CI options for testing non x86_64 architectures.

I can look into something like that. Cross compiling is straightforward. Execution on a runner in a simulator is expected to be slow.

As for the original topic in this PR, using the compiler flag -gnu++11 or similar does activate libquadmath. But I do find some other interesting problems. I'll try to sort out what's all happening and report soon.

The original point should be fixed in the header. But I find unrelated potential issues.

@jzmaddock
Copy link
Collaborator

@ckormanyos : Please be aware that the drone CI is still cycling it's first run and is showing up some new failures (mostly Jamfile errors spotted so far), I'll get to those later when the whole run has completed.

@ckormanyos
Copy link
Member Author

Please be aware that the drone CI is still cycling it's first run and is showing up some new failures (mostly Jamfile errors spotted so far)

Understood. Let's see how it goes.

The changes in this particular branch here are limited in scope. So uless some major problems arise in drone, I'll simply keep this branch synchronized with drone's progress and we can handle the merge of the PR as we deem fit after that all stabilizes.

@ckormanyos
Copy link
Member Author

ckormanyos commented Feb 5, 2021

Reformulate the change request of issue #506

  1. Selected functions like complex log, sqrt, pow, etc. to handle zeros, NaNs and INFs better (or less wrongly).
  2. Thereby, let C99 Annex G be the guide to those special values resulting from (1).
  3. Retain the individual specializations for float, double and long double.
  4. Additional overloads, insofar as needed, could be handled with enable_if now that C++11 is used in Math.
  5. Use constexpr (in the form of BOOST_CXX14_CONSTEXPR) declarations in front of selected [member]-functions where appropriate.
  6. Check to potentially eliminate dependency on <boost/math/constants.hpp>.

@jzmaddock
Copy link
Collaborator

One other suggestion - we should scatter some BOOST_CXX14_CONSTEXPR declarations in front of the [member]-functions that are constexpr in C++20.

@ckormanyos
Copy link
Member Author

One other suggestion - we should scatter some BOOST_CXX14_CONSTEXPR declarations in front of the [member]-functions that are constexpr in C++20.

Right, as does C++14 <complex> itself. I have modified (via edit) the list above.
Thanks John.

I would also like to see if there is a way to eliminate the dependency on <boost/math/constants.hpp>, which I think is rreally only used for the constant value of log(10).

@ckormanyos
Copy link
Member Author

ckormanyos commented Feb 25, 2021

I will be picking up work on this issue again. In the intermediate time, a lot of progress was made on CI, and the migrations to C++11 in Math and Multiprecision. So I'll also be reviewing the top-level requirements of this issue and simlutaneously attempting to ensure that the direction of progress is consistent with the new stands of Math and Multiprecision.

@ckormanyos
Copy link
Member Author

This PR leaves <boost/cstdfloat.hpp> and its complex functions (also 128-bit) in generally a better state than before and addresses the original point and a few more edge cases. There are probably a few more edge cases undiscovered. But I'm merging this, as it's gotten better and going green on CI.

@ckormanyos ckormanyos merged commit 6a23658 into develop Aug 31, 2021
@ckormanyos ckormanyos mentioned this pull request Nov 5, 2021
@NAThompson NAThompson deleted the i506_cstdfloat_complex_std branch January 24, 2024 21:34
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.

3 participants