[tmva] Check for existence of xml node before reading it

[tmadlener]

This Pull request:

Adds a check for the existence of UseOffsetOrNot fields in TMVA files before trying to read from it.

Changes or fixes:

This avoids breaking the reading of pre-existing TMVA weight files that have been written without this tag. See also #4141 (comment)

Checklist:

• tested changes locally
• updated the docs (not necessary)

I have a small-ish (13K) TMVA weight file that triggered the issue for me originally. Should I add that to some test to ensure that it remains readable? If yes, could you point me to where it should go?

[guitargeek]

Thanks for the PR!

I have a small-ish (13K) TMVA weight file that triggered the issue for me originally. Should I add that to some test to ensure that it remains readable? If yes, could you point me to where it should go?

Absolutely welcome! The tests go to tmva/tmva/test, and are implemented using googletest and declared in CMake with the ROOT_ADD_GTEST macro. An example are the unit tests in TestRandomGenerator.cxx:
https://github.com/root-project/root/blob/master/tmva/tmva/test/CMakeLists.txt#L13

Maybe you can implement a new regression test, like TestRegression.cxx that collects the tests for the cases that regressed at some point, like yours?

[guitargeek]

There are some failures on the CI:

 /github/home/ROOT-CI/src/tmva/tmva/src/VariableNormalizeTransform.cxx: In member function ‘virtual void TMVA::VariableNormalizeTransform::ReadFromXML(void*)’:
 ##[error]/github/home/ROOT-CI/src/tmva/tmva/src/VariableNormalizeTransform.cxx:381:15: error: request for member ‘HasAttr’ in ‘TMVA::gTools’, which is of non-class type ‘TMVA::Tools&()’
     if (gTools.HasAttr(trfnode, "UseOffsetOrNot")) {
   

[github-actions]

Test Results

    21 files      21 suites   3d 14h 1m 35s ⏱️
 3 791 tests  3 791 ✅ 0 💤 0 ❌
77 770 runs  77 770 ✅ 0 💤 0 ❌

Results for commit 950956f.

♻️ This comment has been updated with latest results.

[tmadlener]

Fixed the small typo I introduced via the web editor and things should compile now. I have added a very minimal regression test that I confirmed to fail without the fixes. Let me know if you think this is enough, or if the tests should be adapted somehow.

[guitargeek]

Thanks a lot! Squash and merge maybe, with the commit message of the second original commit?

[andresailer]

I think this needs to be backported to 6.38?

[guitargeek]

You're right. Thanks!

[guitargeek]

Here is the backport, which will make it into 6.38.02:

• [v638] Check for existence of xml node before reading it #20867