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

Incorrect nextafter #798

Open
libbooze opened this issue Jan 18, 2025 · 2 comments
Open

Incorrect nextafter #798

libbooze opened this issue Jan 18, 2025 · 2 comments
Labels
Boost Review Collected Comments from Boost Review Period Bug Something isn't working

Comments

@libbooze
Copy link

libbooze commented Jan 18, 2025

I have no proof that my expectation of what nextafter should do is correct, since PDF document never specifies behavior of this function for decimals, but I believe behavior as currently implemented is incorrect.

For this test code:

template<typename T>
void next_after()
{
    std::print("{}\n", boost::typeindex::type_id<T>().pretty_name());
    const T zero(0.0);
    const T one(1.0);
    constexpr auto after_fn = []
    {
        if constexpr (std::floating_point<T>)
        {
            return [](const T& a, const T& b)
            {
                return std::nextafter(a, b);
            };
        } else
        {
            return [](const T& a, const T& b) {return boost::decimal::nextafter(a,b);};
        }
    }();
    const auto next_zero = after_fn(zero, one);
    using nl = std::numeric_limits<T>;
    std::print("denorm min   : {:.30e}\n", nl::denorm_min());
    std::print("min          : {:.30e}\n", nl::min());

    std::print("next after 0 : {:.30e}   {}\n", next_zero, next_zero == zero);
    T one_k(1024);
    T two_k(2048);
    const auto next_1k = after_fn(one_k, two_k);
    std::print("           1k: {:.30e}\n", one_k);
    std::print("next after 1k: {:.30e}  {}\n\n", next_1k, next_1k == one_k);
}

(if constexpr logic is not really necessary, I just wanted to show I am calling correct functions instead of hitting some issues with that)

prints:

float
denorm min   : 1.401298464324817070923729583290e-45
min          : 1.175494350822287507968736537222e-38
next after 0 : 1.401298464324817070923729583290e-45   false
           1k: 1.024000000000000000000000000000e+03
next after 1k: 1.024000122070312500000000000000e+03   false

double
denorm min   : 4.940656458412465441765687928682e-324
min          : 2.225073858507201383090232717332e-308
next after 0 : 4.940656458412465441765687928682e-324   false
           1k: 1.024000000000000000000000000000e+03
next after 1k: 1.024000000000000227373675443232e+03   false

boost::decimal::decimal32
denorm min   : 1.000000000000000000000000000000e-101
min          : 1.000000000000000000000000000000e-95
next after 0 : 1.000000000000000000000000000000e-06   false
           1k: 1.024000000000000000000000000000e+03
next after 1k: 1.024000000000000000000000000000e+03   true

boost::decimal::decimal64
denorm min   : 1.000000000000000000000000000000e-398
min          : 1.000000000000000000000000000000e-383
next after 0 : 1.000000000000000000000000000000e-15   false
           1k: 1.024000000000000000000000000000e+03
next after 1k: 1.024000000000000000000000000000e+03   true

boost::decimal::decimal128
denorm min   : 1.000000000000000000000000000000e-6176
min          : 1.000000000000000000000000000000e-6143
next after 0 : 1.000000000000000000000000000000e-33   false
           1k: 1.024000000000000000000000000000e+03
next after 1k: 1.024000000000000000000000000000e+03   true

I have used this commit because before there were some issues with printing
commit e7d36aa24aae68f1cc0b3c2bc23e78dd8099c17d (HEAD -> limits, origin/limits)

Here I believe few things are incorrect, but they all stem from the fact nextafter just adds/substracts epsilon.
First value greater than 0 should be smallest value > 0, not 0 + eps.
For 1k case because epsilon is smallest increment possible over 1.0 it is so small that when added to 1024 value remains 1024.

@mborland
Copy link
Member

If you pull in the changes to limits is this still an issue? I wonder if this is a second order effect of epsilon being off

@libbooze
Copy link
Author

If you pull in the changes to limits is this still an issue? I wonder if this is a second order effect of epsilon being off

As I said I think epsilon should not be used for nextafter. But to answer your question: I am on limits branch, commit mentioned in original comment.

@mborland mborland added Bug Something isn't working Boost Review Collected Comments from Boost Review Period labels Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Boost Review Collected Comments from Boost Review Period Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants