SND-LHC / advsndsw

Software for AdvSND
1 stars 3 forks source link

added exit point #9

Closed NayanaBangaru closed 2 months ago

NayanaBangaru commented 2 months ago

Tried to add the exit point. Empty leaves in root file.

olantwin commented 2 months ago

Empty tree or empty leaf?

olantwin commented 2 months ago

Can't reproduce the issue, the ExitPoint leaf seems to be filled.

Not sure though, whether I like having a TVector3 as a class member or whether it would be better to add just the three coordinates as leaves.

olantwin commented 2 months ago

CC @siilieva @dcentanni

I would suggest that we keep the meaning of fX,fY,fZ the same as other FairMCPoints, to not risk breaking code elsewhere.

To have methods to return exit and entry points, we need to store at least three additional coordinated, doesn't matter whether this is entry or exit, so that we can provide methods to access both.

I would probably lean towards storing the three coordinates as scalar leaves, but we can provide methods to return TVector3s.

NayanaBangaru commented 2 months ago

Alright. I would keep the meaning of fX, fY, and fZ then. I can also add the three coordinates for each entry and exit point as leaves instead of TVector3s.

siilieva commented 2 months ago

mmm, I'd use TVector3 for all coordinates - it'd be confusing to have scalars and TVector3 for positions in the same class added: I totally agree on having all three types of coordinates : mean, entry, exit

olantwin commented 2 months ago

mmm, I'd use TVector3 for all coordinates - it'd be confusing to have scalars and TVector3 for positions in the same class added: I totally agree on having all three types of coordinates : mean, entry, exit

I think generally vectors indeed would be nicer, but currently FairMCPoints and as a result all of our MC points use scalars for the positions, so it might be better to keep these consistent. I have provided accessors though, so that vectors can easily be retrieved for convenience.

siilieva commented 2 months ago

mmm, I'd use TVector3 for all coordinates - it'd be confusing to have scalars and TVector3 for positions in the same class added: I totally agree on having all three types of coordinates : mean, entry, exit

I think generally vectors indeed would be nicer, but currently FairMCPoints and as a result all of our MC points use scalars for the positions, so it might be better to keep these consistent. I have provided accessors though, so that vectors can easily be retrieved for convenience.

I see, good. It confused me FairMCPoints uses scalar class members for position, but TVector3 for the default constructor.

siilieva commented 2 months ago

I read many comments, overall everything it is fine by me at least from the coding pov. I don't get what convention on either entering or exiting position did we end up to

In the end, we'll have the mean position(as before) and will add the exit pos to the point class. The latter will feature a function to return the entry pos as well (entry pos will not a data member) Entry+Exit coordinates are needed for the simulation of det. response.

olantwin commented 2 months ago

@siilieva Then I'll just make the names consistent and it's good to go?

siilieva commented 2 months ago

@siilieva Then I'll just make the names consistent and it's good to go?

ok, as I mentioned I dont insist on the case/format choice

olantwin commented 2 months ago

Used the ROOT/Taligent convention for now. If everything looks OK, I will autosquash the fixups and merge.