rat-pac / ratpac-two

Open source edition of RAT-PAC
Other
11 stars 23 forks source link

Have PMTInfo account for Local Offset #144

Closed JamesJieranShen closed 1 month ago

JamesJieranShen commented 1 month ago

If the mother volume of the PMT arrays have an offset with respect to the world volume, pmtinfo will record pmt positions based on the mother volume's coordinate system. This patch accounts for it by:

@llebanowski -- this should address some of the offsets you are seeing when using the centroid fit processor (among any others).

Potentially breaking.

llebanowski commented 1 month ago

Since local_offset is not a required argument in AddPMT(), it seems safest to initialize it to zero.

JamesJieranShen commented 1 month ago

Since local_offset is not a required argument in AddPMT(), it seems safest to initialize it to zero.

local_offset is a stack variable so initialization should be done automatically, based on TVector3's default constructor https://root.cern.ch/doc/master/TVector3_8h_source.html#l00247.

Worth noting, also, that local_offset is one value shared across all PMTs in the geometry, so it should not be allowed to be set at AddPMT(). DS::PMTInfo is only created in PMTArrayFactory and PMTCoverageFactory. Local offset is now correctly set in PMTArrayFactory. It should not be needed for PMTCoverageFactory. Although I have not seen this class being used ever so maybe there's unanticipated results with its usage.

llebanowski commented 1 month ago

Right, so it is initialized to zero.
To generalize the application, could this be added to PMTFactoryBase instead of PMTArrayFactory?
Thanks

JamesJieranShen commented 1 month ago

Right, so it is initialized to zero. To generalize the application, could this be added to PMTFactoryBase instead of PMTArrayFactory? Thanks

It is a bit harder to implement, but in fact doesn't seem impossible. I can do some testing tomorrow.

JamesJieranShen commented 1 month ago

@llebanowski I just re-implemented the fix in a different way. There's no more local_offset field in DS::PMTInfo. This field didn't make sense in the case where you have PMT arrays in different mother volumes (example in mind is the OD/ID PMTs in SuperK).

The local_offset correction should now be applied for any PMTFactories. It may still break if the mother volume of a PMTArray is rotated, but nobody has been crazy enough to do that yet. Somebody better at SO(3) can take a crack at it;)