Closed ryanelandt closed 1 year ago
Great PR it is awesome to see the formal correctness of the types get even closer to those of built-ins.
Thanks Ryan (@ryanelandt)
@jzmaddock I can add super-simple test case(s) for these if you think that's a good idea and handle this PR. Thoughts?
@jzmaddock I can add super-simple test case(s) for these if you think that's a good idea and handle this PR. Thoughts?
Yes please! My apologies for being AWAL recently, hopefully I'll start catching up soon...
A trivial change to test_numeric_limits.cpp would do it.... we also need to fix up at least gmp.hpp and mpfr.hpp which have the same mistakes.
trivial change to test_numeric_limits.cpp would do it.... we also need to fix up at least gmp.hpp and mpfr.hpp which have the same mistakes
Great. I can try this (if I may), ... get back into the game. Some of the recent stuff around here has been a bit too challenging for me. This one seems like a good one I could try.
Cc: @jzmaddock and @ryanelandt
@ckormanyos feel free to make the necessary changes.
Hi John (@jzmaddock) and Ryan (@ryanelandt) before I get started adding tests and the other two backends, a question, ...
Does the presence of (now or soon-to-be) non-zero denorm_min()
mean that has_denorm
needs to be changed to something other than std::denorm_absent
?
presence of (now or soon-to-be) non-zero
denorm_min()
mean thathas_denorm
needs to be changed
Ahhh OK, it is clear from the original post. My bad, ... it is clear from the description use has_denorm
as absent and then denorm_min()
is min()
.
Hi Ryan (@ryanelandt)
Cc: @jzmaddock
changes also to MPFR and GMP
And mpfi
. I had also forgotten a few needed (min)()
values. CI is cycling again and looking better...
OK we have the change set, it's green on the new tests.
Ryan (@ryanelandt) and John (@jzmaddock) if you see anything i missed or suggest more testing depth, I'd be happy to go another round. Otherwise, form my side, this one is ready to merge.
Ryan (@ryanelandt) thanks again and super good catch!
Thank you for completing this PR for me. I think it looks good. I have a few questions about the unit tests.
Why use BOOST_TEST(a == b)
instead of BOOST_CHECK_EQUAL(a, b)
? Is this to avoid needing to have print support for the quantities being tested?
Could for example:
BOOST_TEST(!(std::numeric_limits<Number>::denorm_min() > (std::numeric_limits<Number>::min)()));
BOOST_TEST(!(std::numeric_limits<Number>::denorm_min() < (std::numeric_limits<Number>::min)()));
Be replaced with:
BOOST_TEST(std::numeric_limits<Number>::denorm_min() == (std::numeric_limits<Number>::min)());
The latter in shorter, it also gives the expected result in the cases that (std::numeric_limits<Number>::min)()
is NaN
.
Is the check that:
BOOST_TEST(FP_ZERO != (boost::math::fpclassify)(std::numeric_limits<Number>::denorm_min()));
necessary? Because of the check 2 lines above it, I think it's effect is to check if BOOST_TEST(FP_ZERO != FP_NORMAL)
.
BTW, based on local tests, this PR breaks this test in Boost Math.
BTW, based on local tests, this PR breaks
Then we can't merge it. I can look tomorrow. Maybe we need help from Matt (@mborland)?
I made a fix for the Boost Math side.
made a fix
Cool. I'd say handle the fix in Math, then we return here. Thanks
Thanks Chris and Ryan!!
Other than a few minor comments above, this looks good to me.
Other than a few minor comments above, this looks good to me
Thank you John.
This is a silly question, one that I've had for a while. Does anyone know how to simply accept all the suggested comments as a batch change that leads to a single renewed CI run? Or do I/we have to manually change all the codes? Rookie question, I know.
OK I can handle all these review suggestions. Please give me a day or so to do the editing and run CI.
BTW, what is the reason that some of the sets of tests in test_cpp_bin_float.cpp
seem to be disabled for ASAN and UBSAN?
@ckormanyos there is a lot of code duplication testing things for both denorm_min
and min_val
. You might consider putting these tests in a lambda function that looks something like:
const auto fn_small_val_tests = [&](const T x){
// Shared tests go here using x instead of `denorm_min` and `min_val`
};
fn_small_val_tests(denorm_min);
fn_small_val_tests(min_val);
might consider putting these tests in a lambda function
Good catch Ryan, surely would be better. But, ... I've got refactoring goals on basically all files in Math, Multiprecision and in all of Boost for that matter.
So my basic philosophy in the test files, which are known by us to be very non-modernized, is, ... leave it until we have a chance to do a dedicated refactor in something like a GSoC.
The failure is on a clone attempt on drone. Merging.
@ckormanyos Thanks for getting this PR in!
A type
T
is specialized ifstd::numeric_limits<T>::is_specialized
is true. For specialized floating point types that support denorms,denorm_min
should be less thanmin
. For specialized floating point types that don't support denorms,denorm_min
should be equal tomin
, e.g., see this or this.The types
cpp_bin_float
andcpp_dec_float
are floating point types that do not support denorms. This PR changes the values ofdenorm_min
for these types from0
tomin
.