Adding FSD convertor to analysis tree

added 1 commit

651f1483 - apply clang format

Compare with previous version

added 39 commits

651f1483...df38a54b - 36 commits from branch computing:master
832cc18a - First running version of FsdHit and FsdModule converter in AT
b0c060e0 - renaming variables in CbmFsdHitsConverter.cxx
f7477a23 - apply clang format

Compare with previous version

@dvorar10_AT_fjfi.cvut.cz, @f.kornas,

the crash of the macro happens in line 71 of analysis/common/analysis_tree_converter/CbmRecEventHeaderConverter.cxx

rec_event_header_->SetField(GetPsdEnergy(event), iEpsd_);

Since I am not sure if the AnalysisTree converter should support PSD and FSD could you please have a look at the problem. This MR is currently blocking the creation of the release candidate branch.

If I understood @f.kornas correctly he is already testing the code, so tehre must be a working version.

@f.uhlig Hi Florian, since the FSD was shifted by 10cm, it should be possible to have both FSD and PSD within the same setup. I will download the code and try to understand the problem.

@f.uhlig So I managed to run this version, having both fsd and psd in the setup. However, I was using the v23a geometries for fsd and pipe (our standard testing version which is currently not available from the official geometry directory, so I copied them by hand). Then I tried to crosscheck with fsd v23h and pipe v21h geometries, but currently I get a crash in the digitization stating that [FATAL] FsdDigitize: parameter values for FSD digitization does not meet a sanity check!! This I did not test before, so I cannot tell you more now, just that removing the psd does not solve this problem (psd version was always v22a).

When I run "make test" in the build directory, one of the tests is failing, i.e. 41/134 Test #41: run_s100e_reco_ts_eb_ideal .........................***Failed Required regular expression not found. Regex=[Macro finished successfully ] 45.71 sec but I am not sure that this has something to do with fsd at converter!?

How can I actually reproduce this error message in the CbmRecEventHeaderConverter?

@f.uhlig,

the test appears in the normal test chain. If you run cmake with the option -DCBM_TEST_MODEL=Nightly the tests are created. Simply change into the macro/C2F directory in your build folder and execute make test. This will run the tests with the defined parameters in the correct sequence.

Then your setup is different from the one used by the CI and the one used by me. Do you have some changes to the sis100_electron setup?

Ah, ok, I have identified the issue, its the run_analysis_tree_maker.C macro (which we had a chat already today earlier on), which is - to my knowledge - not used by anyone and therefore outdated.

We can discuss whether this can be removed, but since there have not been so dramatic changes, I will try to update it to the current state, such that we can proceed with the MR.

So the run_analysis_tree_maker.C has been updated and should now be 1:1 compatible with run_analysis_tree_maker_json_config.C (except for the geometry part, because this is read in json approach making use of the transport configuration class).

Still there is one error resulting in a crash which I could not get rid of. Its after the run is executed:

[INFO] MCEvent 1 with T0 1000 [INFO] Writing MC-tracks from event # 0 file 0 terminate called after throwing an instance of 'std::out_of_range' what(): Detector::Channel - wrong channel number 40 Number of channels in this detector is 40

I've tried to fix it for quite some time now, will continue to do so, but some help is very welcome!

@f.kornas,

this is the error I have also seen. I had the feeling that this is somehow related to the coexistence of PSD and FSD. What are the number of channels in the PSD and what are the number of channels for the FSD?

Still I wonder, because in principle FSD should not be used in the test as it is based on the sis100_electron setup and this does not know about FSD. What I can crosscheck is to produce some reco file with my json chain and then try to run and compare the 2 AT converter macros...

I could localize the problem, it appears in

https://git.cbm.gsi.de/computing/cbmroot/-/blob/fb5077586572badc1ce0978c6c34d5cf63f05022/analysis/common/analysis_tree_converter/CbmPsdModulesConverter.cxx#L65

where we have 40 PSD modules but inside the loop over PSD hits, the Hit Module ID reaches 41 and then it crashes. What is strange, that if I run with my json approach, the counter goes up to 412 modules and then this problem doesn't show up.

The problem is not in the run_analysis_tree_maker.C, because when I use input from my json approach (tra, digi, reco), everything runs smoothly.

When I use transport + parameter + geometry files from the CI test and run the rest via json, I get the same error message: 'std::out_of_range' what(): Detector::Channel - wrong channel number 40 Number of channels in this detector is 40

@f.uhlig The transport files should be generated by the first CI test based on macro/C2F/c2f_transport.C. I don't know who is using this macro and maintaining the code, maybe this does not fit with the recent developments related to FSD?

In the first test of this C2F folder, when c2f_transport.C is executed, there is a strange messange, when the setup is resistered:

-E- registerSetup: Unknown module ID 13

appears right after FSD is loaded (and afterwards there is only plattform). Apart from that, everything looks healthy.

FSD is not present in the transport output of this first step. I will try to fix it.

Ok. This might explain the problem. Nevertheless if a detector is not present it should not be used in any later stage. This is something we should check.

I don't know, whether this is the origin of the issue, I am just trying to scan for problems/differences to the working test...

As an update: I found that within the CbmPsdHitsConverter.cxx when looping over the Psd Hits, the Hit module ID has 40 entries going from 1 to 44 (modules 19,20,25 and 26 are missing). As the module number is set by the counter i as "i+1", this might not cause the problem of exceeding 40, however, also use just "i" as a module number does not fix the problem.

If I change "hit->GetModuleID()" to "i" in this line: https://git.cbm.gsi.de/computing/cbmroot/-/blob/master/analysis/common/analysis_tree_converter/CbmPsdModulesConverter.cxx#L65 then the crash is avoided, but I am not sure if that is a proper solution.

Apart from this issue: But for the CI test I get another crash then, related to the missing FSD.

@f.uhlig If I remove the FSD part from macro/analysis/common/analysis_tree_converter/run_analysis_tree_maker.C, all tests pass successfully.

So remaining questions: (1) Is the solution for the PSD modules a proper one? (2) Would it be fine to leave the run_analysis_tree_maker.C without FSD? Otherwise all the previous macros (transport, digi, reco) involved in the C2F CI test need to be adapted too which might take me some time to get everything working and I don't know if that's needed?

@f.kornas,

first of all thanks for your work. Concerning your questions I have no really good answers.

(1) If the solution fixing the PSD is the proper one I can't answer. If so this would imply that the current implementation was wrong. I expect that the PWGs should have seen the error before. Beside that the problem only pops up with the FSD detector added. So in my opinion the problem is some strange interference between FSD and PSD which I honestly don't understand.

(2) I think this is a decision of the PWGs if they need the FSD in the AnalysisTree or not. I understood it in the way that the addition of the FSD was a requirement for the release.

The C2F Ci tests should be adapted any way. I think they should reflect what you do use for your normal production as much as possible. I have also changes for the chain with the fixed MVD digitization and cluster finder. In the C2F chain the old macro run_reco_event.C is used instead of run_reco.C which is used in the macro/run CI chains. I had to add the ideal event builder to run_reco_event.C since otherwise the mvd wasn't working properly.

Actually now I found the line where the error message is produced, its in the AnalysisTree package: https://github.com/HeavyIonAnalysis/AnalysisTree/blob/master/core/Detector.hpp#L62

I will try to understand the logic behind and to derive a fix for this problem...

That's why I couldn't find the error in CbmRoot. I forgot to check the AnalysisTree package.

Just an intermediate update: It seems that there is a problem when the channels are filled: When I call GetNumberOfChannels from the AnalysisTree::DetectorModules psd_modules_ object, (1) in the C2F CI test this number is 40 which results in the crash as the when the channel is called by moduleID > 40, it does not exist. (2) in the running json chain, it properly set to 412 and therefore no problem appears.

However, in both cases n_psd_modules is 412 before the channels are added, so the origin of the problem should be actually here: https://git.cbm.gsi.de/computing/cbmroot/-/blob/master/analysis/common/analysis_tree_converter/CbmPsdModulesConverter.cxx#L52

Could it be that this is simply a stupid mistake. If I am correct the FSD code is based on the existing PSD code.

The error appeared when the FSD code was added, so could it be that in the FSD code by mistake some PSD parameters are set?

I don't think so, as the FSD Converter task is always called after the PSD. I currently checking the changes to CbmConverterManager.cxx as there was some change also affecting the PSD code.

Summary of the current findings/open issues:

There are two behaviors I don't quite understand at the moment.

Before FSD was introduced, the number of PSD modules was read from the DataHeader and found to be 40. Accordingly, 40 channels are initialized and even though the module ID goes up to 44 (skipping 19,20,25,26; no idea why) the code runs without error.

Now after FSD was introduced, the number of modules as read from AnalysisTree::DataHeader is 412 (which is all PSD + FSD modules), however, in this line https://git.cbm.gsi.de/computing/cbmroot/-/blob/master/analysis/common/analysis_tree_converter/CbmPsdModulesConverter.cxx#L52 it magically knows that only 40 modules correspond to PSD, such that afterwards a call psd_modules_->GetNumberOfChannels() returns 40. Now again when looping the PSD hits, the module ID runs up tp 44 and when it exceeds 40, the code crashes returning this message: https://github.com/HeavyIonAnalysis/AnalysisTree/blob/master/core/Detector.hpp#L62 so obviously the input number to the Channel() function reached equal value compared to GetNumberOfChannels().

So what I currently don't understand:

(1) Why it crashes now when the module ID exceeds 40, while before it was no problem at all? According to the code it looks like the proper behavior, but why it worked before?

(2) Why PSD modules 19,20,25 and 26 are not present in PsdHit->GetModuleID()?

(3) How does the AddChannel function knows that out of all 412 modules, only 40 correspond to PSD?

(2) Not completely sure (would have to recheck from which index and which corner they start counting), but I suspect the "missing" modules are the ones in the beampipe hole, as at some point there where simulations without hole for physics/Geant tuning. Something like (one module is not there in each corner from what I remember):

    1| 2| 3| 4             1| 2| 3| 4
 5| 6| 7| 8| 9|10       5| 6| 7| 8| 9|10
11|12|13|14|15|16      11|12|13|14|15|16      
17|18|19|20|21|22  ==> 17|18|--|--|21|22
23|24|25|26|27|28  ==> 23|24|--|--|27|28
29|30|31|32|33|34      29|30|31|32|33|34
35|36|37|38|39|40      35|36|37|38|39|40
   41|42|43|44            41|42|43|44

mentioned in merge request !1489 (merged)

mentioned in merge request !1533 (merged)

As the changes have been already merged, this MR can be closed.

This merge request was superseded by !1533 (merged) which introduces some bugfixes to run all tests successful.

Close this merge request.

closed

Adding FSD convertor to analysis tree

Merge request reports

Activity