FairRootGroup
Add support for TVirtualMCSensitiveDetector.
Inherit FairModule from TVirtualMCSensitiveDetector, which allows to use VMC stepping. Tested with Tutorial1. Requires changes in detector interface.
Could you please summarize shortly what is the idea behind this change? Will it be backward compatible/optional or will it lead to deep changes?
It is still work in progress. Idea is to simplify handling of sensitive detectors in FairRoot. Currently foreseen as a default solution. Would require changes in FairDetector interface, which are not backward compatible. Changes are in ProcessHits method.
@MohammadAlTurany , Looking into it again, I have realised that the experiments will need to change ProcessHits method in every detector class
The declaration:
virtual Bool_t ProcessHits(FairVolume* v = 0) = 0;
will be changed to:
virtual void ProcessHits()=0;
Looking at the code, you've also moved the function from FairDetector to FairModule. What is the reason to remove its argument and return value? Can't it stay it used to be?
This change is needed only if you went to use the sensitive detector or did I miss something here?
I thought there was a wrapper in FairRoot which calls the old function if the new one isn't available.
@karabowi We used to take care about bookkeeping of sensitive volumes in FairRoot. Idea of this PR is to let VMC do it. The new function is VMC requirement. @MohammadAlTurany I thought we want to get read of the parts where we have to handle sensitive volumes on our own completely. Does it make sense to keep both options? I agree to Florian that a wrapper can help here. But, we have to be aware, that FairVolume parameter of the overloaded methods might have been used in the experiment code. How do we handle this with the wrapper?
One could get the current volume and the corresponding FairVolume and call the old method, at least for some time to allow the experiments to move smoothly to the new one!
With the current state of this branch, there seems to be no any MC points stored in the output files.
@karabowi thanks for testing. Probably something went wrong while rebasing. The whole thing will be revisited completely. I will open another PR soon.
I started testing the Pull Request for CbmRoot and had several problems which are spread over the whole application stack. For my tests I used FairSoft apr21p2 and FairRoot v18.6.7 as baseline since these are the default versions used by CbmRoot. To compare to the PR I cherry-picked the changes from the PR into v18.6.7.
Compilation of the patches FairRoot as well as CbmRoot worked but when running out transport tests all of them failed with an assertion in the Initialize() function of one of our detector classes. The assertion checked that the function was only called once but obviously it was called twice. When deriving FairModule from TVirtualMCSensitiveDetector the Initialize() function of Detectors/Modules is called from VMC and we need to remove calling the function from out FairMCApplication. After the change in FairRoot and several fixes in CbmRoot I was able to run the tests without errors. When comparing the results produced with the patched and unpatched versions they were identical for most of our detector systems but for two of them only part of the MCPoints were produced. After two days of debugging I probably found the problem in TGeant3. If I am correct the issue is already fixed with v4-0 which is the default for FairSoft apr22. I am currently compiling the new FairSoft version to confirm that with this new version the problem is fixed.
Preliminary comparison of execution times of the patched and unpatched FairRoot versions don't show any observable difference.
@fuhlig1 Could you add this double-calling-prevention assert to some of the FairRoot tests/examples please? Maybe open it in a new PR, so that we get this test before we merge the TVirtualMCSensitiveDetector PR?
@ChristianTackeGSI,
the requested test case is described in #1195 and implemented with #1196.
@kresan,
sorry but I think I messed up the PR a minute ago. There was a conflict which I tried to solve using the web editor but it seems to me that instead off doing a rebase the webpage did a merge. Could you please have a look.
@fuhlig1, I reverted back your merge action. Now we need to resolve conflicts properly.
@kresan,
thanks. Could you solve the conflicts. I fear if I do it from the web frontend we run into the same problem.
We noticed in this morning's meeting, that we should consider raising the minimum required geant3 version to the one containing some fixes relevant to this pull request.
If we do so, it should be done in the CMakeLists.txt.
Also before we merge this one, we probably should do a rebase/cleanup of the commits.
And before we merge it, we should get clear on our release planning (this currently targets 19.2, not 19.0).