root icon indicating copy to clipboard operation
root copied to clipboard
verified publisher icon root-project

[math] Fix debug assertion failure

Open bellenot opened this issue 1 year ago • 8 comments

Fix the following error (visible in TMVA):

libTMVA: Debug Assertion Failed!
         Expression: cannot seek vector iterator before begin

bellenot avatar Dec 03 '24 11:12 bellenot

Test Results

    18 files      18 suites   3d 19h 43m 28s ⏱️  2 678 tests  2 678 ✅ 0 💤 0 ❌ 46 484 runs  46 484 ✅ 0 💤 0 ❌

Results for commit 5a6ed9bc.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Dec 03 '24 14:12 github-actions[bot]

@pcanal thanks for the review! I'll let @lmoneta take the final decision

bellenot avatar Dec 04 '24 08:12 bellenot

@pcanal , @lmoneta So with Philippe's version:

C:\root-dev\build\x64\release\tmva\tmva\test>Release\stressTMVA.exe -b
******************************************************************
* TMVA - S T R E S S and U N I T test suite (FAST)
******************************************************************
Event [107/107]..................................................OK
VariableInfo [31/31].............................................OK
DataSetInfo [20/20]..............................................OK
DataSet [15/15]..................................................OK
Factory [5/11].................................................FAIL
Reader [2/2].....................................................OK
ReaderMT [100/100]...............................................OK
CutsGA [3/3].....................................................OK
LikelihoodD [3/4]..............................................FAIL
PDERS [4/4]......................................................OK
PDEFoam [3/4]..................................................FAIL
KNN [3/4]......................................................FAIL
Fisher [3/4]...................................................FAIL
BoostedFisher [3/4]............................................FAIL
LD [3/4].......................................................FAIL
MLP [3/4]......................................................FAIL
MLPBFGS [3/4]..................................................FAIL
SVM [3/4]......................................................FAIL
BDTG [4/4].......................................................OK
BDT [3/4]......................................................FAIL
DNN CPU [4/4]....................................................OK
Regression_LD [4/4]..............................................OK
Regression_MLPBFGSN [4/4]........................................OK
Regression_BDTG2 [4/4]...........................................OK
IPythonInteractive [2/2].........................................OK
Event [107/107]..................................................OK
VariableInfo [31/31].............................................OK
DataSetInfo [20/20]..............................................OK
DataSet [15/15]..................................................OK
Factory [5/11].................................................FAIL
Reader [2/2].....................................................OK
ReaderMT [100/100]...............................................OK
CutsGA [3/3].....................................................OK
LikelihoodD [3/4]..............................................FAIL
PDERS [4/4]......................................................OK
PDEFoam [3/4]..................................................FAIL
KNN [3/4]......................................................FAIL
Fisher [3/4]...................................................FAIL
BoostedFisher [3/4]............................................FAIL
LD [3/4].......................................................FAIL
MLP [3/4]......................................................FAIL
MLPBFGS [3/4]..................................................FAIL
SVM [3/4]......................................................FAIL
BDTG [4/4].......................................................OK
BDT [3/4]......................................................FAIL
DNN CPU [4/4]....................................................OK
Regression_LD [4/4]..............................................OK
Regression_MLPBFGSN [4/4]........................................OK
Regression_BDTG2 [4/4]...........................................OK
IPythonInteractive [2/2].........................................OK
******************************************************************
*  SYS: Windows_NT AMD64 Family 25 Model 80 Stepping 0, AuthenticAMD
******************************************************************
*  CPUTIME   =  23.6   *  Root6.35.01   20241105/0
******************************************************************

And with my version:

C:\root-dev\build\x64\release\tmva\tmva\test>Release\stressTMVA.exe -b
******************************************************************
* TMVA - S T R E S S and U N I T test suite (FAST)
******************************************************************
Event [107/107]..................................................OK
VariableInfo [31/31].............................................OK
DataSetInfo [20/20]..............................................OK
DataSet [15/15]..................................................OK
Factory [11/11]..................................................OK
Reader [2/2].....................................................OK
ReaderMT [100/100]...............................................OK
CutsGA [3/3].....................................................OK
LikelihoodD [4/4]................................................OK
PDERS [4/4]......................................................OK
PDEFoam [4/4]....................................................OK
KNN [4/4]........................................................OK
Fisher [4/4].....................................................OK
BoostedFisher [4/4]..............................................OK
LD [4/4].........................................................OK
MLP [4/4]........................................................OK
MLPBFGS [4/4]....................................................OK
SVM [4/4]........................................................OK
BDTG [4/4].......................................................OK
BDT [4/4]........................................................OK
DNN CPU [4/4]....................................................OK
Regression_LD [4/4]..............................................OK
Regression_MLPBFGSN [4/4]........................................OK
Regression_BDTG2 [4/4]...........................................OK
IPythonInteractive [2/2].........................................OK
Event [107/107]..................................................OK
VariableInfo [31/31].............................................OK
DataSetInfo [20/20]..............................................OK
DataSet [15/15]..................................................OK
Factory [11/11]..................................................OK
Reader [2/2].....................................................OK
ReaderMT [100/100]...............................................OK
CutsGA [3/3].....................................................OK
LikelihoodD [4/4]................................................OK
PDERS [4/4]......................................................OK
PDEFoam [4/4]....................................................OK
KNN [4/4]........................................................OK
Fisher [4/4].....................................................OK
BoostedFisher [4/4]..............................................OK
LD [4/4].........................................................OK
MLP [4/4]........................................................OK
MLPBFGS [4/4]....................................................OK
SVM [4/4]........................................................OK
BDTG [4/4].......................................................OK
BDT [4/4]........................................................OK
DNN CPU [4/4]....................................................OK
Regression_LD [4/4]..............................................OK
Regression_MLPBFGSN [4/4]........................................OK
Regression_BDTG2 [4/4]...........................................OK
IPythonInteractive [2/2].........................................OK
******************************************************************
*  SYS: Windows_NT AMD64 Family 25 Model 80 Stepping 0, AuthenticAMD
******************************************************************
*  CPUTIME   =  23.8   *  Root6.35.01   20241105/0
******************************************************************

bellenot avatar Dec 04 '24 13:12 bellenot

@pcanal @lmoneta ping

bellenot avatar Dec 17 '24 21:12 bellenot

@lmoneta When the function was changed to return the 'correct' value in case the search for value is smaller than anything in the collection, stressTMVA fails (see https://github.com/root-project/root/pull/17179#issuecomment-2517437510). Can you confirm that this is indeed the correct behavior (as opposed to the test 'accepting' unintentionally the wrong result?

pcanal avatar Dec 18 '24 21:12 pcanal

@pcanal, I am not sure what the correct behaviour is. I think the way is used (at least in TMVA) is to get the index of the closest value in a collection (e.g. a set of points in a TGraph). For example, doing: int index = std::distance(x.begin(),TMath::BinarySearch(x.begin(),x.end(),value)) Returning the interator to end(), also for the case the search value is on the lower side, might be confusing.
The tests fail because now the found index is the last one instead of the first one. Of course, this could be corrected in the code.

What I don't like now is that Iterator TMath::BinarySearch(x.begin(),x.end(), value) returns x.first() for values x < x[0], while int Math::BinarySesrch(x.size(), x.data(), value) returns -1 for values x < x[0]

Maybe we should clearly note this in the documentation

lmoneta avatar Dec 18 '24 22:12 lmoneta

Returning the interator to end(), also for the case the search value is on the lower side, might be confusing.

However this is the C++ way to return an invalid value. The old code was return an invalid address (but not in a way the caller could tell).

The tests fail because now the found index is the last one instead of the first one.

It is supposed to be one past the last one (per se). If the test and/or the code used by the test checking for invalid return values, i.e. handling the case where the search failed?

pcanal avatar Dec 18 '24 23:12 pcanal

I would propose to do the following:

  • change the return type of the function to be an integer, so it is consistent with the other TMath::BinarySearch function and returning -1 or n depending if the value is smaller or larger than the collection range. By doing this users will get a compiler error and then will fix the error accordingly without getting a different behaviour without noticing it

lmoneta avatar Dec 19 '24 10:12 lmoneta

Superseded by:

  • https://github.com/root-project/root/pull/19143

guitargeek avatar Jun 23 '25 19:06 guitargeek