Draft : Unpacker trd2d
First version of the TRD2D unpacker following the new scheme.
Depends on !415 (merged)
Merge request reports
Activity
mentioned in merge request !415 (merged)
Hi Alex, I will include your draft into my branch just now, thanks a lot for preparing it.
View minor comments what might be problematic with this MR:
- You removed the hardcoded 3 in run_unpack_tsa for the TRD. This was there on purpose right now I only provided parameter labeled with run 3. I am a bit hesitant to flood our parameter repository now with 1500 parameter files, without really knowing what runs to look at. Since, currently also we not yet have real automatic run by run parameters. I will include a safety into the automatic scheme, but this requires some time, since, my todo list is quite long right now.
- I do not think we should put that many printouts by default (especially without using LOG(...) to indicate a debug severity) into the CbmRecoUnpack loop. When really looping over big amounts of data, they are anyhow not looked at anymore.
- If you don't mind I would add Algo to your unpacking algo class name, such that it is clear what is what.
Hi Pascal,
Thx for your quick reaction. No problem with all 3 suggestions of yours. The only problem that I have right now is the usage of the same container named TrdDigi like in simulation. The problem was that I couldn't just get the pointer to the fOutputVec without creating a new one which is overwriting the one created by the TRD (see void CbmTrdUnpackConfig2D::Init(FairRootManager ioman)). This was also the reason for making the
void Init(FairRootManager ioman) virtual. If you see a better solution please feel free to apply itHi Alex, I included the TRD2D algo/config into the scheme see !415 (merged) I also implemented a workaround for the issue with the TrdDigi branch. Now the user can decide in the macro by a setter to the config, if he/she wants to give special branchnames instead of the default ones, e.g. SpadicTrdDigi. So it is possible to access the digis in a separate way (good for first debugging I would say). If no exztra branchname is passed I skip the branch registration for the 2D config and it gets the pointer to the output branch from the TRD1D config during the Init stage.
@a.bercuci please have a look at !415 (merged) . If you are fine with the implemetation of the Fasp algo and config, please withdraw this (!416 (closed) ) MR since, it would create massive merge conflicts with !415 (merged) . Otherwise, let me know what changes you require. If you agree with !415 (merged) you also do not need to fix the clang-format here.
-
Hi @praisig , So in principle I see that you clean the code. Sorry for not doing it myself but I was not aware that we have to fulfill all requirements. I assumed running the unpackers together was the priority ! Nevertheless I will check myself the code in the future. Sorry for the time you have spend.
Besides I have only formal requests :
-
I would like to change in the run_unpack_tsa the name from FaspTrdDigi to TrdDigiFasp. It would also be fair to change in the case of the SPADIC read-out the name accordingly. As far as I am aware the naming convention for classes is "name[specialization]". If you add Fasp as specialization of the TrdDigi than Spadic is also on the same position ! I agree that nomenclature might be superfluous for the data read-out but on a longer run I think it will matter.
-
I understand that the modification in the CbmRecoUnpack will allow to produce the digis under the same Output vector name as it is in simulation. Otherwise we will not be able to use the reconstruction immediately as it will need quite some modifications as you may check for yourself
In conclusion the merge request on my own will be removed
-
