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.

Sign up for GitHub

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

Jump to bottom

[RF] RooProdPdf test failure on s390x #12430

Closed
1 task done
ellert opened this issue Mar 5, 2023 · 0 comments · Fixed by #12447
Closed
1 task done

[RF] RooProdPdf test failure on s390x #12430

ellert opened this issue Mar 5, 2023 · 0 comments · Fixed by #12447
Assignees
@guitargeek
Labels
affects:master affects:6.28 bug in:RooFit/RooStats
Projects
Fixed in 6.30/00

Comments

@ellert
Copy link
Contributor

ellert commented Mar 5, 2023

  • Checked for duplicates

Describe the bug

The gtest-roofit-roofitcore-test-testRooProdPdf test fails on s390x.
This is a big-endian architecture.

See the log below.
The assert does not fail because the two values are the same with differnt byte order. The issue is more complicated than that::

$ printf '%x\n%x\n' 3649468902 2448666198
d98679e6
91f3ae56

Expected behavior

Passing test on s390x

To Reproduce

  1. Build root for s390x
  2. Run unit tests

Setup

  1. ROOT version: 6.28.00
  2. Operating system: Fedora for s390x, RHEL+EPEL for s390x
  3. How you obtained ROOT: Compilation from source

Additional context

Thefailing test:

 295/1272 Test  #250: gtest-roofit-roofitcore-test-testRooProdPdf .........................***Failed    1.02 sec
Running main() from /builddir/build/BUILD/googletest-1.13.0/googletest/src/gtest_main.cc
Note: Google Test filter = -RCsvDS.Remote:RRawFile.Remote:RSqliteDS.Davix:TFile.ReadWithoutGlobalRegistrationWeb:TFile.ReadWithoutGlobalRegistrationNet:RNTuple.TClassEBO
[==========] Running 8 tests from 2 test suites.
[----------] Global test environment set-up.
[----------] 2 tests from RooProdPdf
[ RUN      ] RooProdPdf.TestGetPartIntList
/builddir/build/BUILD/root-6.28.00/roofit/roofitcore/test/testRooProdPdf.cxx:146: Failure
Expected equality of these values:
  hashRooProduct(prod)
    Which is: 3649468902
  2448666198
[  FAILED  ] RooProdPdf.TestGetPartIntList (229 ms)
[ RUN      ] RooProdPdf.TestDepsAreCond
Warning in <RooNaNPacker>: Fast recovery from undefined function values only implemented for little-endian machines. If necessary, request an extension of functionality on https://root.cern
[       OK ] RooProdPdf.TestDepsAreCond (644 ms)
[ DISABLED ] RooProdPdf.DISABLED_ChangeServerNormSetForProdPdfInAddPdf
[----------] 2 tests from RooProdPdf (873 ms total)
[----------] 6 tests from RooProdPdf/TestProdPdf
[ RUN      ] RooProdPdf/TestProdPdf.CachingOpt/opt0off
[       OK ] RooProdPdf/TestProdPdf.CachingOpt/opt0off (17 ms)
[ RUN      ] RooProdPdf/TestProdPdf.CachingOpt/opt0cpu
[       OK ] RooProdPdf/TestProdPdf.CachingOpt/opt0cpu (9 ms)
[ RUN      ] RooProdPdf/TestProdPdf.CachingOpt/opt1off
[       OK ] RooProdPdf/TestProdPdf.CachingOpt/opt1off (13 ms)
[ RUN      ] RooProdPdf/TestProdPdf.CachingOpt/opt1cpu
[       OK ] RooProdPdf/TestProdPdf.CachingOpt/opt1cpu (9 ms)
[ RUN      ] RooProdPdf/TestProdPdf.CachingOpt/opt2off
[       OK ] RooProdPdf/TestProdPdf.CachingOpt/opt2off (16 ms)
[ RUN      ] RooProdPdf/TestProdPdf.CachingOpt/opt2cpu
[       OK ] RooProdPdf/TestProdPdf.CachingOpt/opt2cpu (9 ms)
[----------] 6 tests from RooProdPdf/TestProdPdf (75 ms total)
[----------] Global test environment tear-down
[==========] 8 tests from 2 test suites ran. (949 ms total)
[  PASSED  ] 7 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] RooProdPdf.TestGetPartIntList
 1 FAILED TEST
  YOU HAVE 1 DISABLED TEST
CMake Error at /builddir/build/BUILD/root-6.28.00/cmake/modules/RootTestDriver.cmake:232 (message):
  error code: 1
@guitargeek guitargeek changed the title RooFit test failure on s390x [RF] RooProdPdf test failure on s390x Mar 7, 2023
guitargeek added a commit to guitargeek/root that referenced this issue Mar 7, 2023
@guitargeek
[RF] Rewrite RooProdPdf.TestGetPartIntList unit test
2c3e927 
In the ROOT 6.26 development cycle, the RooProdPdf was partially
rewritten in moden C++ with less manual memory allocation to improve
performance (PR root-project#7907).

In that PR, a unit test that verifies the RooProdPdf can correctly deal
with factorizing PDFs was implemented. However, that test used an
arbitrary PDF where the correct factorization was checked in a rather
crude way: check by hashing the content of the RooProdPdf cache element
for a given normalization set that said PR doesn't change any behavior
(the reference hash was hardcoded in the unit test).

This commit suggests a better alternative for the unit test, checking
for a multidimensional product pdf of factorizing uniform pdfs that the
pdf values for differenc normalization sets is as expected. This should
cover the same functionality and is less fragile and implementation
dependend than hashing the cache elements.

This closes GitHub issue root-project#12430, as the rewritten test is not affected
anymore by the problem reported in that issue.
@guitargeek guitargeek mentioned this issue Mar 7, 2023
Merged
@guitargeek guitargeek linked a pull request Mar 7, 2023 that will close this issue
[RF] Rewrite RooProdPdf.TestGetPartIntList unit test #12447
Merged
guitargeek added a commit that referenced this issue Mar 7, 2023
@guitargeek
[RF] Rewrite RooProdPdf.TestGetPartIntList unit test
c200042 
In the ROOT 6.26 development cycle, the RooProdPdf was partially
rewritten in moden C++ with less manual memory allocation to improve
performance (PR #7907).

In that PR, a unit test that verifies the RooProdPdf can correctly deal
with factorizing PDFs was implemented. However, that test used an
arbitrary PDF where the correct factorization was checked in a rather
crude way: check by hashing the content of the RooProdPdf cache element
for a given normalization set that said PR doesn't change any behavior
(the reference hash was hardcoded in the unit test).

This commit suggests a better alternative for the unit test, checking
for a multidimensional product pdf of factorizing uniform pdfs that the
pdf values for differenc normalization sets is as expected. This should
cover the same functionality and is less fragile and implementation
dependend than hashing the cache elements.

This closes GitHub issue #12430, as the rewritten test is not affected
anymore by the problem reported in that issue.
@guitargeek guitargeek added this to Issues in Fixed in 6.30/00 via automation Mar 7, 2023
guitargeek added a commit to guitargeek/root that referenced this issue Mar 31, 2023
@guitargeek
[RF] Rewrite RooProdPdf.TestGetPartIntList unit test
6fa9c0b 
In the ROOT 6.26 development cycle, the RooProdPdf was partially
rewritten in moden C++ with less manual memory allocation to improve
performance (PR root-project#7907).

In that PR, a unit test that verifies the RooProdPdf can correctly deal
with factorizing PDFs was implemented. However, that test used an
arbitrary PDF where the correct factorization was checked in a rather
crude way: check by hashing the content of the RooProdPdf cache element
for a given normalization set that said PR doesn't change any behavior
(the reference hash was hardcoded in the unit test).

This commit suggests a better alternative for the unit test, checking
for a multidimensional product pdf of factorizing uniform pdfs that the
pdf values for differenc normalization sets is as expected. This should
cover the same functionality and is less fragile and implementation
dependend than hashing the cache elements.

This closes GitHub issue root-project#12430, as the rewritten test is not affected
anymore by the problem reported in that issue.
guitargeek added a commit to guitargeek/root that referenced this issue Mar 31, 2023
@guitargeek
[RF] Rewrite RooProdPdf.TestGetPartIntList unit test
189aed0 
In the ROOT 6.26 development cycle, the RooProdPdf was partially
rewritten in moden C++ with less manual memory allocation to improve
performance (PR root-project#7907).

In that PR, a unit test that verifies the RooProdPdf can correctly deal
with factorizing PDFs was implemented. However, that test used an
arbitrary PDF where the correct factorization was checked in a rather
crude way: check by hashing the content of the RooProdPdf cache element
for a given normalization set that said PR doesn't change any behavior
(the reference hash was hardcoded in the unit test).

This commit suggests a better alternative for the unit test, checking
for a multidimensional product pdf of factorizing uniform pdfs that the
pdf values for differenc normalization sets is as expected. This should
cover the same functionality and is less fragile and implementation
dependend than hashing the cache elements.

This closes GitHub issue root-project#12430, as the rewritten test is not affected
anymore by the problem reported in that issue.
guitargeek added a commit that referenced this issue Mar 31, 2023
@guitargeek
[RF] Rewrite RooProdPdf.TestGetPartIntList unit test
c512572 
In the ROOT 6.26 development cycle, the RooProdPdf was partially
rewritten in moden C++ with less manual memory allocation to improve
performance (PR #7907).

In that PR, a unit test that verifies the RooProdPdf can correctly deal
with factorizing PDFs was implemented. However, that test used an
arbitrary PDF where the correct factorization was checked in a rather
crude way: check by hashing the content of the RooProdPdf cache element
for a given normalization set that said PR doesn't change any behavior
(the reference hash was hardcoded in the unit test).

This commit suggests a better alternative for the unit test, checking
for a multidimensional product pdf of factorizing uniform pdfs that the
pdf values for differenc normalization sets is as expected. This should
cover the same functionality and is less fragile and implementation
dependend than hashing the cache elements.

This closes GitHub issue #12430, as the rewritten test is not affected
anymore by the problem reported in that issue.
omazapa pushed a commit to omazapa/root that referenced this issue Apr 13, 2023
@guitargeek @omazapa
[RF] Rewrite RooProdPdf.TestGetPartIntList unit test
5f72686 
In the ROOT 6.26 development cycle, the RooProdPdf was partially
rewritten in moden C++ with less manual memory allocation to improve
performance (PR root-project#7907).

In that PR, a unit test that verifies the RooProdPdf can correctly deal
with factorizing PDFs was implemented. However, that test used an
arbitrary PDF where the correct factorization was checked in a rather
crude way: check by hashing the content of the RooProdPdf cache element
for a given normalization set that said PR doesn't change any behavior
(the reference hash was hardcoded in the unit test).

This commit suggests a better alternative for the unit test, checking
for a multidimensional product pdf of factorizing uniform pdfs that the
pdf values for differenc normalization sets is as expected. This should
cover the same functionality and is less fragile and implementation
dependend than hashing the cache elements.

This closes GitHub issue root-project#12430, as the rewritten test is not affected
anymore by the problem reported in that issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@guitargeek guitargeek

Labels
affects:master affects:6.28 bug in:RooFit/RooStats
Projects
No open projects
Fixed in 6.30/00
Issues
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[RF] Rewrite RooProdPdf.TestGetPartIntList unit test guitargeek/root
2 participants
@ellert @guitargeek