-
Notifications
You must be signed in to change notification settings - Fork 76
Add an automatic voxelisation option to PhotonVisibilityService #135
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
Add an automatic voxelisation option to PhotonVisibilityService #135
Conversation
…undaries and then optimising for the cryostat Also added the option to save lists of voxel IDs within the TPC and outside
A new Pull Request was created by @bwynneHEP (Benjamin Wynne) for develop. It involves the following packages: larsim @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
The code-checks are being triggered in jenkins. |
-code-checks
Then commit the changes and push them to your PR branch. |
Pull request #135 was updated. @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please check and sign again. |
The code-checks are being triggered in jenkins. |
+code-checks |
@bwynneHEP, thanks for the PR. We are trying to understand the motivation for these changes--could you give us some background information? Presenting this at a LArSoft Coordination Meeting could be helpful. Regarding the |
@knoepfel the motivation is simply convenience. The context is generating samples for the "photon library," but perhaps this has more general application. We want a voxel grid that covers the cryostat efficiently, with boundaries that align to the TPC so that it is easy to label a voxel as inside or outside the TPC. All these dimensions can be looked up, and a voxelisation calculated and added to a configuration file, but it's far quicker and more reliable to just calculate it directly in the PVS from the existing geometry description. Also this includes a simple optimisation step that would otherwise add complexity to the apparently back-of-an-envelope guesswork used before. The output lists of voxel indices inside or outside the TPC then simplifies sorting jobs and outputs for large-scale production of samples. |
I should also note that all this behaviour defaults to off - there is no modification to existing workflows. Any effects would only be felt at configuration time, if the behaviour is enabled. |
@bwynneHEP, sorry for the delay, and thanks for the response. The idea seems reasonable. I will send some implementation suggestions later this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwynneHEP, I've added some implementation suggestions for your consideration. I'm happy to discuss any of them with you.
Pull request #135 was updated. @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please check and sign again. |
Pull request #135 was updated. @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please check and sign again. |
The code-checks are being triggered in jenkins. |
+code-checks |
Apologies for spam, minor git fail. Comments should now be incorporated, thanks @knoepfel |
trigger build |
The tests are being triggered in jenkins. |
+LArSoft tests OK on slf7 for c14:prof |
+LArSoft tests OK on slf7 for e26:prof |
-uboone tests failed on slf7 for e26:prof |
The uBOONE fails seem to be unrelated to code changes
|
-sbnd tests warning, with build warning,, with ignored warning for build, on slf7 for e26:prof |
-icarus tests warning on slf7 for e26:prof |
-dune tests tests warning on slf7 for e26:prof |
Approved -- looks okay and defaults to no change in workflows. |
approve |
This pull request is fully signed and it will be merged to develop and built in the next LArSoft release after it passes the integration tests. |
Added an automatic voxelisation option, fitting exactly to the TPC boundaries and then optimising for the cryostat
Also added the option to save lists of voxel IDs within the TPC and outside, to allow efficient generation of photon library.
NB, this seems to require a rebuild of larana, specificially due to
https://github.com/LArSoft/larana/blob/b6abb28a9515bb5a8193e6aa2fd131a2ee0ecd8e/larana/OpticalDetector/SimPhotonCounter_module.cc#L192
This seems like a problem with the build system, but maybe I'm missing something obvious.