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

3 tests fail #3739

Open
yurivict opened this issue Nov 14, 2022 · 12 comments
Open

3 tests fail #3739

yurivict opened this issue Nov 14, 2022 · 12 comments
Labels
type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances

Comments

@yurivict
Copy link

          Start  356: NumberToString.DecimalNotationUpTo21Digits
 360/2979 Test  #356: NumberToString.DecimalNotationUpTo21Digits .............................................................***Failed    0.09 sec
Running main() from /wrkdirs/usr/ports/devel/googletest/work/googletest-release-1.12.1/googletest/src/gtest_main.cc
Note: Google Test filter = NumberToString.DecimalNotationUpTo21Digits
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from NumberToString
[ RUN      ] NumberToString.DecimalNotationUpTo21Digits
/disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.2.1/Modules/Core/Common/test/itkNumberToStringGTest.cxx:87: Failure
Expected equality of these values:
  numberToString(power_of_ten)
    Which is: "100000010000"
  '1' + std::string(exponent, '0')
    Which is: "100000000000"
/disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.2.1/Modules/Core/Common/test/itkNumberToStringGTest.cxx:88: Failure
Expected equality of these values:
  numberToString(-power_of_ten)
    Which is: "-100000010000"
  "-1" + std::string(exponent, '0')
    Which is: "-100000000000"
/disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.2.1/Modules/Core/Common/test/itkNumberToStringGTest.cxx:96: Failure
Expected equality of these values:
  numberToString(power_of_ten)
    Which is: "0.000009999999999999999"
  "0." + std::string(-1 - exponent, '0') + '1'
    Which is: "0.00001"
/disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.2.1/Modules/Core/Common/test/itkNumberToStringGTest.cxx:97: Failure
Expected equality of these values:
  numberToString(-power_of_ten)
    Which is: "-0.000009999999999999999"
  "-0." + std::string(-1 - exponent, '0') + '1'
    Which is: "-0.00001"
[  FAILED  ] NumberToString.DecimalNotationUpTo21Digits (0 ms)
[----------] 1 test from NumberToString (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (0 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] NumberToString.DecimalNotationUpTo21Digits

 1 FAILED TEST
          Start 1201: itkTIFFImageIOPlannarConfig2
1205/2979 Test #1201: itkTIFFImageIOPlannarConfig2 ...........................................................................***Exception: SegFault  0.08 sec
Trying reader->Update()             
          Start 1204: itkTIFFImageIOInfoTest2
1208/2979 Test #1204: itkTIFFImageIOInfoTest2 ................................................................................***Exception: SegFault  0.08 sec

Version: 5.2.1
clang-14
FreeBSD 13.1

@yurivict yurivict added the type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances label Nov 14, 2022
@dzenanz
Copy link
Member

dzenanz commented Nov 14, 2022

@N-Dekker If I remember correctly, you might be fresh with NumberToString?

@N-Dekker
Copy link
Contributor

@yurivict Interesting 🤔 Regarding the itkNumberToStringGTest failures: Did you possibly do anything to tweak the floating point behavior of the compiler? As in https://clang.llvm.org/docs/UsersManual.html#controlling-floating-point-behavior ?

@N-Dekker
Copy link
Contributor

@yurivict It would be helpful if you could reproduce those float-to-string conversion errors (for example from 100000000000.0 to "100000010000" by using the double-conversion library from https://github.com/google/double-conversion directly (without ITK). Doing something like the following, but outside of ITK:

template <typename TValue>
std::string
FloatingPointNumberToString(const TValue val)
{
// Declare a buffer, large enough for strings like:
// "-100000000000000000000" (-1e20, either float or double, 23 chars)
// "-1.7976931348623157e+308" (-DBL_MAX, 25 chars)
// "-0.0000033333333333333333" (-3e-005/9.0, 26 chars)
char buf[32];
double_conversion::StringBuilder builder(buf, sizeof(buf));
if (!ConvertToShortest(double_conversion::DoubleToStringConverter::EcmaScriptConverter(), val, builder))
{
itkGenericExceptionMacro(<< "Conversion failed for " << val);
}
return std::string(builder.Finalize());
}

Would you be able to?

@yurivict
Copy link
Author

The conversion prints: 100000000000.0 -> 100000000000:

#include <double-conversion/double-to-string.h>
#include <iostream>

bool
ConvertToShortest(const double_conversion::DoubleToStringConverter & converter,
                  const float                                        val,
                  double_conversion::StringBuilder &                 builder)
{
  // Call the converter member function that is specific for single-precision `float`.
  return converter.ToShortestSingle(val, &builder);
}


template <typename TValue>
std::string
FloatingPointNumberToString(const TValue val)
{
  // Declare a buffer, large enough for strings like:
  // "-100000000000000000000" (-1e20, either float or double, 23 chars)
  // "-1.7976931348623157e+308" (-DBL_MAX, 25 chars)
  // "-0.0000033333333333333333" (-3e-005/9.0, 26 chars)
  char buf[32];

  double_conversion::StringBuilder builder(buf, sizeof(buf));

  if (!ConvertToShortest(double_conversion::DoubleToStringConverter::EcmaScriptConverter(), val, builder))
    std::cout << "Conversion failed for " << val << std::endl;

  return std::string(builder.Finalize());
}

int main() {
        std::cout << "100000000000.0 -> " << FloatingPointNumberToString<double>(100000000000.0) << std::endl;
}

@N-Dekker
Copy link
Contributor

N-Dekker commented Nov 15, 2022

@yurivict Thanks for trying. 👍 So it looks like converter.ToShortestSingle(val, &builder) works fine. Can you please also try converter.ToShortest(val, &builder)? (ToShortestSingle is specific for 32-bit float, while ToShortest is for 64-bit double.) So just the same test, only without the word "Single" in the code 😃

@yurivict
Copy link
Author

Without Single it prints this:

100000000000.0 -> 99999997952

@N-Dekker
Copy link
Contributor

Sorry, my bad. When you remove "Single", also change the type of the val parameter from float to double, please! Just like the ConvertToShortest overload for a double val, in itkNumberToString.cxx

bool
ConvertToShortest(const double_conversion::DoubleToStringConverter & converter,
const double val,
double_conversion::StringBuilder & builder)
{
return converter.ToShortest(val, &builder);
}

@yurivict
Copy link
Author

100000000000.0 -> 100000000000

@yurivict
Copy link
Author

5.3rc04 has 4 cases failing on FreeBSD 13.1 STABLE:

	129 - ITKMeshInDoxygenGroup (Failed)
	365 - NumberToString.DecimalNotationUpTo21Digits (Failed)
	1264 - itkTIFFImageIOPlannarConfig2 (SEGFAULT)
	1267 - itkTIFFImageIOInfoTest2 (SEGFAULT)

N-Dekker added a commit to N-Dekker/ITK that referenced this issue Nov 17, 2022
Aims to ease the analysis of NumberToStringGTest test failures, like the
`NumberToString.DecimalNotationUpTo21Digits` failures reported by yurivict at
InsightSoftwareConsortium#3739 saying:

    /disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.2.1/Modules/Core/Common/test/itkNumberToStringGTest.cxx:87: Failure
    Expected equality of these values:
      numberToString(power_of_ten)
        Which is: "100000010000"
      '1' + std::string(exponent, '0')
        Which is: "100000000000"
@N-Dekker
Copy link
Contributor

N-Dekker commented Nov 17, 2022

@yurivict Can you please check if the NumberToString.DecimalNotationUpTo21Digits failures may be solved, just by adding one extra #include to Modules/Core/Common/test/itkNumberToStringGTest.cxx, as follows:

#include <cmath> // For std::pow.

(At line 22, right after #include <gtest/gtest.h>)

In other words, does pull request #3751 indeed fix those NumberToString.DecimalNotationUpTo21Digits failures?

N-Dekker added a commit to N-Dekker/ITK that referenced this issue Nov 17, 2022
Addressed `NumberToString.DecimalNotationUpTo21Digits` failures reported by yurivict at
InsightSoftwareConsortium#3739 saying:

    /disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.2.1/Modules/Core/Common/test/itkNumberToStringGTest.cxx:87: Failure
    Expected equality of these values:
      numberToString(power_of_ten)
        Which is: "100000010000"
      '1' + std::string(exponent, '0')
        Which is: "100000000000"

It appears that without `#include <cmath>`, the wrong `pow` overload may be called.

Also explicitly declared the local `power_of_ten` variables as `TValue` and
prevented a narrowing conversion from those `std::pow` calls, just to be sure.
@yurivict
Copy link
Author

After more modules were enabled more tests are now failing: test.log

N-Dekker added a commit to N-Dekker/ITK that referenced this issue Nov 20, 2022
Aims to ease the analysis of NumberToStringGTest test failures, like the
`NumberToString.DecimalNotationUpTo21Digits` failures reported by yurivict at
InsightSoftwareConsortium#3739 saying:

    /disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.2.1/Modules/Core/Common/test/itkNumberToStringGTest.cxx:87: Failure
    Expected equality of these values:
      numberToString(power_of_ten)
        Which is: "100000010000"
      '1' + std::string(exponent, '0')
        Which is: "100000000000"
dzenanz pushed a commit that referenced this issue Nov 29, 2022
Aims to ease the analysis of NumberToStringGTest test failures, like the
`NumberToString.DecimalNotationUpTo21Digits` failures reported by yurivict at
#3739 saying:

    /disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.2.1/Modules/Core/Common/test/itkNumberToStringGTest.cxx:87: Failure
    Expected equality of these values:
      numberToString(power_of_ten)
        Which is: "100000010000"
      '1' + std::string(exponent, '0')
        Which is: "100000000000"
@N-Dekker
Copy link
Contributor

@yurivict Now that PR #3751 is merged, can you please rerun the NumberToString GoogleTest? PR #3751 might not have fixed the issue, but it should at least provide some more information. Specifically: do the failures occur with 32-bit float, or 64-bit double, or both?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances
Projects
None yet
Development

No branches or pull requests

3 participants