Fix root GetSeed #1412

fuhlig1 · 2023-06-22T16:26:21Z

This PR brings first a test which checks if the set seed value is properly stored in FairBasParSet container during the simulation. Due to a breaking change in ROOT 6.24 the return value of the function TRandom3::GetSeed() doesn't return the seed value but a fixed value of 624. The newly added test should fail for FairSoft versions apr22 and nov22.
The PR will be updated with the proper fix for the problem in a second step.

Checklist:

[x ] Followed the Contributing Guidelines

examples/simulation/Tutorial2/macros/compare_seed_value.C

examples/simulation/Tutorial2/macros/CMakeLists.txt

examples/simulation/Tutorial2/macros/run_tutorial2.C

examples/simulation/Tutorial2/macros/compare_seed_value.C

ROOT changes the return value of the GetSeed function of TRandom3. FairRoot uses this function to write the seed value used in the simulation to the FairBaseParSet parameter container for later usage. With ROOT > 6.24 the saved value is wrong due to the change in the ROOT code. Add a test which fails if the used seed value isn't properly stored in the parameter container.

Between Root 6.22 and 6.24 the return value of TRandom3::GetSeed() was changed. Since Root 6.24 the function GetSeed returns the current element of the state used to generate the random number which returns a fixed value of 624 for TRandom3. To get the seed value which was set by the function SetSeed one neeeds to use the GetSeed() function from the base class (gRandom->TRandom::GetSeed()), FairSoft apr21p2 does not have the problem but any later FairSoft version will save the wrong information in the parameter container FairBaseParSet irrespective of the seed value defined in Root. This commit fixes the issue and saves the correct seed value specified by gRandom->SetSeed(<your value>).

fuhlig1 · 2023-06-23T10:45:14Z

When adding the FairRoot header files in the macro as well as adding the needed path information to CPATH such that the FairRoot headers are found I get the following compilation error

Processing /opt/fairroot/source/dev/examples/simulation/Tutorial2/macros/run_tutorial2.C()...
G__Base dictionary payload:3692:23: error: cannot initialize a parameter of type 'TObject *' with an lvalue of type 'FairGenerator *'
fGenList->Add(generator);
^~~~~~~~~
/opt/fairsoft/nov22p1/include/root/TObjArray.h:68:34: note: passing argument to parameter 'obj' here
void Add(TObject *obj) { AddLast(obj); }
^
Assertion failed: (!Calls.empty() && "Missing lambda call operator!"), function getLambdaCallOperator, file DeclCXX.cpp, line 1397.

The code is in FairPrimaryGenerator and compiles without problems. Does anybody has any idea what could be the problem.

examples/simulation/Tutorial2/macros/create_digis.C

ChristianTackeGSI · 2023-06-23T11:27:19Z

Does anybody has any idea what could be the problem.

I looked into this a few months ago.

My conclusion at that time was:

We have the headers baked into the distionary (where the root interpreter loads them all at once when loading the shared library)
We have the headers in the filesystem, where the #include finds them.

At that point I tried to disable the embedding of the headers in the dictionary. But that did not work out nicely either. As you already notice, this requires the include paths to be correct.

But afterwards I had some issues with circular includes (A.h includes B.h, B.h includes A.h). With our current design these circular includes are not trivial to avoid always.

examples/simulation/Tutorial2/macros/CMakeLists.txt

examples/simulation/Tutorial2/macros/create_digis.C

examples/simulation/Tutorial2/macros/compare_seed_value.C

examples/simulation/Tutorial2/macros/CMakeLists.txt

fuhlig1 · 2023-06-26T06:58:55Z

Who should resolve the conversations when an issue was fixed? I would think this should be done by the person who opened the conversation and requested the change. Is this the correct assumption?

ChristianTackeGSI

Could you please fix the formatting errors?

ChristianTackeGSI

Just one other note.

ChristianTackeGSI · 2023-06-27T08:09:40Z

examples/simulation/Tutorial2/macros/compare_seed_value.C

+}
+
+int main() {
+  return compare_seed_value("",1);


Why are you adding this here, if you remove it in a later commit again?

Because it was added by accident. This was only meant for testing and should not end up in the PR.

Can you please remove it from the relevant commit and possibly squash some commits together?

ChristianTackeGSI · 2023-06-27T09:16:42Z

Things look good to me now!

Can you consider squashing some things together? For example the formatting changes could already be in the commits that actually add/change that relevant code.

Don't include the headers when using the cling interpreter. For some not yet understood reason the macro compilation crashes when including the headers with cling,

Both tests ex_compare_seed_value and ex_sim_tutorial2_create_digis depend on the test ex_sim_tutorial2 which is specified by the required fixture. Since both tests also read the same output file they must not run in parallel. This is ensured by the resource lock.

fuhlig1 · 2023-06-27T13:17:30Z

Things look good to me now!

Can you consider squashing some things together? For example the formatting changes could already be in the commits that actually add/change that relevant code.

Done.

ChristianTackeGSI · 2023-06-27T14:54:10Z

(I'll press the merge button, once our CI stuff is fixed.)

fuhlig1 · 2023-09-01T09:25:13Z

@ChristianTackeGSI,

should be ported to v18.8_patches, maybe even to the v18.6_patches

fuhlig1 requested review from dennisklein, karabowi and ChristianTackeGSI June 22, 2023 16:26

fuhlig1 changed the base branch from master to dev June 22, 2023 16:26

fuhlig1 force-pushed the fix_root_getseed branch from 16be03e to f1ff24e Compare June 22, 2023 16:56

ChristianTackeGSI reviewed Jun 22, 2023

View reviewed changes

examples/simulation/Tutorial2/macros/compare_seed_value.C Outdated Show resolved Hide resolved

examples/simulation/Tutorial2/macros/CMakeLists.txt Outdated Show resolved Hide resolved

examples/simulation/Tutorial2/macros/run_tutorial2.C Outdated Show resolved Hide resolved

dennisklein reviewed Jun 22, 2023

View reviewed changes

examples/simulation/Tutorial2/macros/compare_seed_value.C Outdated Show resolved Hide resolved

dennisklein reviewed Jun 22, 2023

View reviewed changes

examples/simulation/Tutorial2/macros/compare_seed_value.C Outdated Show resolved Hide resolved

fuhlig1 added 2 commits June 23, 2023 12:01

fuhlig1 force-pushed the fix_root_getseed branch from e120f4f to d0548eb Compare June 23, 2023 10:04

ChristianTackeGSI reviewed Jun 23, 2023

View reviewed changes

examples/simulation/Tutorial2/macros/create_digis.C Outdated Show resolved Hide resolved

fuhlig1 force-pushed the fix_root_getseed branch from d0548eb to 6f521d1 Compare June 23, 2023 11:41

dennisklein reviewed Jun 23, 2023

View reviewed changes

examples/simulation/Tutorial2/macros/CMakeLists.txt Outdated Show resolved Hide resolved

ChristianTackeGSI reviewed Jun 23, 2023

View reviewed changes

examples/simulation/Tutorial2/macros/create_digis.C Outdated Show resolved Hide resolved

examples/simulation/Tutorial2/macros/compare_seed_value.C Outdated Show resolved Hide resolved

examples/simulation/Tutorial2/macros/CMakeLists.txt Outdated Show resolved Hide resolved

fuhlig1 force-pushed the fix_root_getseed branch 4 times, most recently from 5eeeac8 to 9b7085d Compare June 26, 2023 12:17

ChristianTackeGSI reviewed Jun 27, 2023

View reviewed changes

fuhlig1 added 2 commits June 27, 2023 11:45

Add or update license header and include statements

6355627

Don't include the headers when using the cling interpreter. For some not yet understood reason the macro compilation crashes when including the headers with cling,

fuhlig1 force-pushed the fix_root_getseed branch from 92b255d to 0e9e78e Compare June 27, 2023 10:02

karabowi approved these changes Jun 27, 2023

View reviewed changes

ChristianTackeGSI approved these changes Jun 27, 2023

View reviewed changes

ChristianTackeGSI merged commit 5e885fd into FairRootGroup:dev Jun 27, 2023

fuhlig1 added this to the v18.8 milestone Sep 1, 2023

fuhlig1 deleted the fix_root_getseed branch September 1, 2023 15:02

This was referenced Sep 4, 2023

[backport 18.6] #1244, #1389, #1447 #1448

Merged

Fix license header #1450

Merged

[backport 18.8] #1389, #1412, #1447 #1453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix root GetSeed #1412

Fix root GetSeed #1412

fuhlig1 commented Jun 22, 2023

fuhlig1 commented Jun 23, 2023

ChristianTackeGSI commented Jun 23, 2023

fuhlig1 commented Jun 26, 2023

ChristianTackeGSI left a comment

ChristianTackeGSI left a comment

ChristianTackeGSI Jun 27, 2023

fuhlig1 Jun 27, 2023

ChristianTackeGSI Jun 27, 2023

fuhlig1 Jun 27, 2023

ChristianTackeGSI commented Jun 27, 2023

fuhlig1 commented Jun 27, 2023

ChristianTackeGSI commented Jun 27, 2023

fuhlig1 commented Sep 1, 2023

Fix root GetSeed #1412

Fix root GetSeed #1412

Conversation

fuhlig1 commented Jun 22, 2023

fuhlig1 commented Jun 23, 2023

ChristianTackeGSI commented Jun 23, 2023

fuhlig1 commented Jun 26, 2023

ChristianTackeGSI left a comment

Choose a reason for hiding this comment

ChristianTackeGSI left a comment

Choose a reason for hiding this comment

ChristianTackeGSI Jun 27, 2023

Choose a reason for hiding this comment

fuhlig1 Jun 27, 2023

Choose a reason for hiding this comment

ChristianTackeGSI Jun 27, 2023

Choose a reason for hiding this comment

fuhlig1 Jun 27, 2023

Choose a reason for hiding this comment

ChristianTackeGSI commented Jun 27, 2023

fuhlig1 commented Jun 27, 2023

ChristianTackeGSI commented Jun 27, 2023

fuhlig1 commented Sep 1, 2023