New attempt to remove shadow variables from UpstreamTaggerHit class

[CelsoFCF]

No description provided.

[olantwin]

This should probably be a const reference.

[CelsoFCF]

It works fine as a pointer.

[olantwin]

It may work, but this way it allows (accidental) modification of the UpstreamTagger by the hit. As there is no good reason I can think of for this to happen, it's useful to have the compiler enforce this by making it const.

Whether it's a const reference or const pointer, I frankly don't care, but conventionally const reference are usually used for this purpose.

[olantwin]

Making the UpstreamTagger and its hits friends might be a better solution than making the members public.

[olantwin]

This inheritance doesn't make sense.

[olantwin]

An UpstreamTagger object as a member of the Hit doesn't make sense. Also, it needs to be //! to make sure it's not serialised.

[CelsoFCF]

The UpstreamTagger object is required to access its members inside GetNode() from UpstreamTaggerHit.cxx

[olantwin]

There are several other ways to do this.
UpstreamTaggerHit is the same format that will be used from data and for storage on disk. Having an UpstreamTagger member of this class (and serialising it), is unnecessary.

Again, I would suggest having a look at the implementation of similar methods for the strawtubes.

[olantwin]

General comment: Please use commit messages.

[CelsoFCF]

I suggest that the pull request be accepted as it is. Then, you can make the small modifications you consider necessary.

[ThomasRuf]

Please have a look what I suggested here: #356 . Until the issue with unique IDs is not fixed, it makes no sense for me to accept anything about the hit class.

[CelsoFCF]

Hi Thomas, I copied that part of the code from TimeDet.cxx. You can access a given module with the GetModule() method (there are 20 different modules) from UpstreamTaggerHit class. Therefore, you can identify each active element of the whole system with a combination of GetGlass() and GetModule() methods. I inform you that I will leave Physics to the industry in the next month. Therefore, I will pass the responsibility of this work to my coleagues Alberto Blanco (alberto@coimbra.lip.pt) and Nuno Leonardo (nuno.leonardo@cern.ch). Best regards, Celso Em 2020-06-08 11:30, Thomas Ruf escreveu:

…

Please have a look what I suggested here: #356 [1] . Until the issue with unique IDs is not fixed, it makes no sense for me to accept anything about the hit class. -- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [2], or unsubscribe [3]. Links: ------ [1] #356 [2] #358 (comment) [3] https://github.com/notifications/unsubscribe-auth/AHHEKMWQBWQ4OODWU5INQNTRVS4VJANCNFSM4NYHRBUA

[ThomasRuf]

Maybe I am missing something. What is created in UpstreamTagger.cxx? Only some unstructured sensitive planes, or a detailed detector layout with sensitive volumes, modules, passive material, support structure etc.
Is the UpstreamTaggerHit class suppossed to make the granulation or is this done in UpstreamTagger?
In any case, the hit class should mimick what is send by the DAQ to the eventbuilder. So, some ADC/TDC value and an unique ID. Definetely not MC position and momentum, and details about the

[ThomasRuf]

detector segmention:

Double_t point_final[3];
const Double_t * mom[3];

Float_t flag;     ///< flag
Float_t t_1,t_2;  ///< TDC on both sides
Int_t RpcModule; //Rpc module
Int_t RpcGlass;  //Rpc glass
Int_t RpcStrip;  //Rpc Strip
Int_t RpcDetector;  //Rpc detector 1 or 2
Int_t Rpc_NeighbourStrip; //Neighbour strip likely to be activated
Double_t det_zdet1;     //!  z-position of veto station

[ThomasRuf]

Assume I read events from a file, and look at an UpstreamTaggerHit object and what to know its xyz position. The method provided for this, TVector3 UpstreamTaggerHit::GetXYZ(), will completely fail. It will call
TGeoNavigator* nav = gGeoManager->GetCurrentNavigator();
TGeoNode* node = GetNode(hit_final, mod);
which then tries to access MC truth information about hit position and particle momentum. This can never work with real data.

[CelsoFCF]

Yes, I see what you mean. Indeed this class can only work with MC data. What I wrote inside GetNode() method had the goal of identifying which module (1 out of 20) and which detector (1 out of 2) received the hit. It also computes the spatial position of the MC hit in the readout strips and tells us which of the strips received the hit. After identifying the active element that received the hit I'm setting the "nav = gGeoManager->GetCurrentNavigator()" to the correct path. If you have some quick suggestion of how to proceed please let me know asap (I have little free time to work on this). Otherwise, we will need to wait for another colleague of mine to take care of this. Best regards, Celso Em 2020-06-08 12:34, Thomas Ruf escreveu:

…

Assume I read events from a file, and look at an UpstreamTaggerHit object and what to know its xyz position. The method provided for this, TVector3 UpstreamTaggerHit::GetXYZ(), will completely fail. It will call TGeoNavigator* nav = gGeoManager->GetCurrentNavigator(); TGeoNode* node = GetNode(hit_final, mod); which then tries to access MC truth information about hit position and particle momentum. This can never work with real data. -- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Links: ------ [1] #358 (comment) [2] https://github.com/notifications/unsubscribe-auth/AHHEKMXP336SXPFFOEOI4H3RVTEDZANCNFSM4NYHRBUA

[CelsoFCF]

I also don't know if we should invest time right now in making the class work for real data. I say this because I think the geometry and properties of the detector will change soon. Em 2020-06-08 13:51, celso escreveu:

…

Yes, I see what you mean. Indeed this class can only work with MC data. What I wrote inside GetNode() method had the goal of identifying which module (1 out of 20) and which detector (1 out of 2) received the hit. It also computes the spatial position of the MC hit in the readout strips and tells us which of the strips received the hit. After identifying the active element that received the hit I'm setting the "nav = gGeoManager->GetCurrentNavigator()" to the correct path. If you have some quick suggestion of how to proceed please let me know asap (I have little free time to work on this). Otherwise, we will need to wait for another colleague of mine to take care of this. Best regards, Celso Em 2020-06-08 12:34, Thomas Ruf escreveu: > Assume I read events from a file, and look at an UpstreamTaggerHit > object and what to know its xyz position. The method provided for > this, TVector3 UpstreamTaggerHit::GetXYZ(), will completely fail. It > will call > TGeoNavigator* nav = gGeoManager->GetCurrentNavigator(); > TGeoNode* node = GetNode(hit_final, mod); > which then tries to access MC truth information about hit position and > particle momentum. This can never work with real data. > > -- > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub [1], or unsubscribe > [2]. > > Links: > ------ > [1] > #358 (comment) > [2] > https://github.com/notifications/unsubscribe-auth/AHHEKMXP336SXPFFOEOI4H3RVTEDZANCNFSM4NYHRBUA

[ThomasRuf]

Could you tell me which is the volume which corresponds to a readout channel? Is it simulated in Geant4 and correspond to a node, or is it something virtuell, only simulated when creating an UpstreamTaggerHit?

The hit class will be used as input for the reconstruction and for modelling the DAQ. It should therefore mimick the reality as much as possible.

[CelsoFCF]

Hi, Everything is simulated in UpstreamTagger.cxx. In UpstreamTaggerHit I'm just accessing information from UpstreamTagger.cxx to learn about which strip and which module was hit. The readout is made from copper strips (either vol_strip or vol_strip64 defined in UpstreamTagger.cxx -> the id of the nodes can be checked there). Em 2020-06-08 15:29, Thomas Ruf escreveu:

…

Could you tell me which is the volume which corresponds to a readout channel? Is it simulated in Geant4 and correspond to a node, or is it something virtuell, only simulated when creating an UpstreamTaggerHit? The hit class will be used as input for the reconstruction and for modelling the DAQ. It should therefore mimick the reality as much as possible. -- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Links: ------ [1] #358 (comment) [2] https://github.com/notifications/unsubscribe-auth/AHHEKMW3MTULHRK3LPRCG6DRVTYS3ANCNFSM4NYHRBUA

[ThomasRuf]

OK, good. Am I right that most of the code in UpstreamTaggerHit::GetNode is a cut and paste of the same code in UpstreamTagger.cxx? Second, you are not using in any way the detID created in UpstreamTagger::ProcessHits. You try to bypass it by using the x,y,z of the point class and re-engineer the name of the detector volume. If you would use properly the detID, UpstreamTaggerHit::GetNode would shrink to two lines. It would not be necessary to make any MC information persistent. If you have a one to one relation, each point object corresponds to a hit object, then the index for a hit object could also be used to access a point object with full MC information.

[CelsoFCF]

Em 2020-06-08 16:21, Thomas Ruf escreveu:

OK, good. Am I right that most of the code in UpstreamTaggerHit::GetNode is a cut and paste of the same code in UpstreamTagger.cxx?

Part of it yes. The rest is an algorithm to use the constructor information to find the path of the active area that was hited. Second, you are not using in any way the detID

created in UpstreamTagger::ProcessHits.

Correct You try to bypass it by using

the x,y,z of the point class and re-engineer the name of the detector volume.

Yes. If you would use properly the detID,

UpstreamTaggerHit::GetNode would shrink to two lines. It would not be necessary to make any MC information persistent. If you have a one to one relation, each point object corresponds to a hit object, then the index for a hit object could also be used to access a point object with full MC information.

I will try to do that but I still have a question. The MC hits are only stored for the active material. However, the strips are not defined as active. Is there a way to store the information about which strips were hited (by all charged particles created by the primary hit in the active glass) or this information can only be obtained at the analysis level?

…

-- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Links: ------ [1] #358 (comment) [2] https://github.com/notifications/unsubscribe-auth/AHHEKMTDE6WGDR32P5IBFTTRVT6XTANCNFSM4NYHRBUA

[ThomasRuf]

I would assume that particles hitting strips will not produce a signal. A signal is caused by particles passing through gas and making ionization, electrons and ions are collected on the strips making an electronic signal. Simulating electrons and ions in Geant4 would be too CPU intensive. This part is then simulated in the digitization step. In order to know which strips are related to which active material, it would be a great advantage if this is encoded in the numbering scheme.
In your detector, what is the relation between strips (readout channels?) and active volume? One active volume is readout by one strip, by many strips, strips are sharing different active volumes, ...?

[CelsoFCF]

Em 2020-06-09 06:28, Thomas Ruf escreveu:

I would assume that particles hitting strips will not produce a signal. A signal is caused by particles passing through gas and making ionization, electrons and ions are collected on the strips making an electronic signal. Simulating electrons and ions in Geant4 would be too CPU intensive. This part is then simulated in the digitization step. In order to know which strips are related to which active material, it would be a great advantage if this is encoded in the numbering scheme. In your detector, what is the relation between strips (readout channels?) and active volume? One active volume is readout by one strip, by many strips, strips are sharing different active volumes, ...?

Giving the most upstream modules as an exemple, the active part of a module consists of three ~2.3m^2 glass plates interspersed with gas gaps. The readout for the whole module is provided by 32 strips (which are located downstream to the whole system).

…

-- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Links: ------ [1] #358 (comment) [2] https://github.com/notifications/unsubscribe-auth/AHHEKMTGJJZOOMVPXLZFSH3RVXB7XANCNFSM4NYHRBUA

[ThomasRuf]

So, do you have a model (from testbeam for example) which describes which strips and what amount of charge is seen for a particle traversing the active part at given x and y?

[ThomasRuf]

This is work in progress. Needs modifications before it can be used in production. Will disable running UpstreamTagger digitization until better solution is found.