Fix root GetSeed#1412
Conversation
16be03e to
f1ff24e
Compare
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>).
e120f4f to
d0548eb
Compare
|
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
The code is in FairPrimaryGenerator and compiles without problems. Does anybody has any idea what could be the problem. |
I looked into this a few months ago. My conclusion at that time was:
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. |
d0548eb to
6f521d1
Compare
|
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? |
5eeeac8 to
9b7085d
Compare
ChristianTackeGSI
left a comment
There was a problem hiding this comment.
Could you please fix the formatting errors?
ChristianTackeGSI
left a comment
There was a problem hiding this comment.
Just one other note.
| } | ||
|
|
||
| int main() { | ||
| return compare_seed_value("",1); |
There was a problem hiding this comment.
Why are you adding this here, if you remove it in a later commit again?
There was a problem hiding this comment.
Because it was added by accident. This was only meant for testing and should not end up in the PR.
There was a problem hiding this comment.
Can you please remove it from the relevant commit and possibly squash some commits together?
|
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.
92b255d to
0e9e78e
Compare
Done. |
|
(I'll press the merge button, once our CI stuff is fixed.) |
|
should be ported to v18.8_patches, maybe even to the v18.6_patches |
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: