Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have PMTInfo account for Local Offset #144

Merged

Conversation

JamesJieranShen
Copy link
Contributor

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:

  • Adding a local_offset file in DS::PMTInfo, meaning that the offset will be recorded in outroot files.
  • Have PMTInfo record the correct position, with respect to the world coordinates. This means that ntuple files as well as rat processors should now have access to the correct pmt positions.

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

Potentially breaking.

@llebanowski
Copy link
Contributor

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

@JamesJieranShen
Copy link
Contributor Author

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
Copy link
Contributor

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

@JamesJieranShen
Copy link
Contributor Author

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 JamesJieranShen force-pushed the hotfix/jierans-pmtinfo-local-offset branch from 4c6807f to 46140db Compare July 18, 2024 18:22
@JamesJieranShen
Copy link
Contributor Author

@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;)

@tannerbk tannerbk merged commit 23fd754 into rat-pac:main Jul 19, 2024
2 checks passed
@JamesJieranShen JamesJieranShen deleted the hotfix/jierans-pmtinfo-local-offset branch December 4, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants