-
Notifications
You must be signed in to change notification settings - Fork 35
PMT encapsulation #256
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
PMT encapsulation #256
Conversation
Hi @LSexton2396 -- thanks for the contribution! I do have some questions about this specific implementation. It appears the code is largely copied from the toroidal PMT construction, and that the difference between the two is just to add a spherical envelope around the PMT. Is this correct? If that is the case, I'm not sure creating a new entire factory is the best approach here. Instead, something similar to how the concentrators are being built seems a better approach. In the
This prevents PMTs from being have to be defined twice (one encapsulated and one not), and makes the code also a lot more flexible since you can now encapsulate every PMT available in ratpac-two. Let me know what you think! |
Hi James, Thanks for the feedback I will look it making some changes. |
please see the |
Hi @JamesJieranShen , it is possible that this example did not fully make it to ratpac2-core. If I grep for add_concentrator nothing comes back in $RATSHARE/src. While it is defined in $RATSHARE/ratdb/SNO/sno.geo and src/geo/src/pmt/PMTConcentrator.cc does exist, there is no link and I can't see the concentrator if I visualize the SNO detector. I think it is simply missing a simple function. |
Thanks for catching that @MarcFBergevin -- do you know where the original code came from? The code should almost certainly live somewhere in https://github.com/rat-pac/ratpac-two/blob/main/src/geo/src/pmt/PMTFactoryBase.cc. However I'm not exactly sure where. Looking at relevant SNO+ code this is where the concetrator build code gets called, although this file has been heavily refactored for that code base. @LSexton2396 if you have access to the repo, perhaps it's worthwhile to take a look:) |
Not 100% sure. That is where it was as well for ratpac-watchman. Let me see if I can find an old example. |
Hi @JamesJieranShen I have made it so the encapsulation is built separately from the pmt. The order is (if encapsulation is turned on): I know have question about copy numbers. The order above means only the encapsulation envelope gets the copy number. In GLG4PMTOpticalModel.cc i see it finds the volume named: "pmtenv" and gets its copy number, I currently have the encapsulation envelope named it. Does it matter if the copynumber is given to the encapsulation which the pmt is inside or does it need to be given to pmt itself? Or is there a better order to do things I am not sure how to give the pmt the id number current ordering? Thanks |
AFAIK the copy number is used when one volume is placed multiple times in the geometry. Are you placing the pmt envelope inside the encapsulation? If that is the case, I don't think pmtenv should have a copy number any more -- the encapsulation should become the mother of the pmt envelope and the envelope should only be placed once. The numbering itself should not matter so keeping the same logic for how the current copy number is assigned to the pmtenv is sufficient. As an aside, the pmtenv is just a big box around the pmt, and is meant to speed up simulation by allowing geant4 to voxelize the detector with larger steps outside of the envelope. So it's not unreasonable for the encapsulation logic to just modify the shape of the envelope itself and make it the correct size and material. However if you would like to keep the envelope and build the encapsulation outside of it that's fine with me as well. |
Without having a volume named "pmtenv" I get the following error and exits: OpticalModelBase::GetPMTID: Cannot decode PMT ID. If i name the the encapsulation volume with pmtenv im corncerned that theyll be no pmt information. So i make the encapsulation once, place the pmt volume inside then place the encapsulation 96 times. But this only runs if I have a volume named pmtenv, and I named the encapsulation volume that so there is a number with each module. My issue is all the pmts id will be 0 so no differring between them? I have the encapsulation within a spherical volume as there are some external object such as metal plates that keep the domes together just makes placing it all easier, so that the whole encapsulation gets rotated/positioned correctly . |
@JamesJieranShen , I think the statement that this is too specific to Button is unfair. The PR now includes switches to turn on and off multiple facet of the encapsulation. Many of these facets are critical for different encapsulation designs. While it would be nice to have this be as general as possible for different types of PMTs, having a working example for one type of PMT is far better than having no implementation at all. The issues you have brought up can be identified and fixed at a later date for individuals that require these features. |
In my opinion, the minimum requirement prior to this being merged is to make this into a standard PMTConstruction, similar to Toroidal and Revolution. The current implementation is somewhat haphazard -- either we stick with EncapsulationPMT being a special type of pmt different from ToroidalPMTs, or we don't. Sure, we can always change this in the future, but the current implementation unnecessarily steps outside of the structure set up in the current code and would hence result in significant technical debt if this were to be merged as-is. This is very likely (at least partially) a result of my suggestions of generalizing the implementation earlier, and I apologize if this has caused any repetition of work. Whether this is specific to BUTTON or not, I don't have a very strong opinion and I'm happy to not die on this hill. This obviously depends on the opinions of @LSexton2396 and other authors to this PR, among other constraints. I'm happy to give my OK once the abovementioned structural changes have been addressed. |
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.
Hi @LSexton2396, I had a quick chat with @MarcFBergevin about this PR again, and it looks like I had a misunderstanding of how the code has been changed due to some leftover files in the code from the old implementation. Thanks for all the changes, this was a lot of great work. I apologize for any confusions I've caused.
Please see above comments for some small changes.
ratdb/ENCAPSULATION.ratdb
Outdated
{ | ||
name: "ENCAPSULATION", | ||
index : "BUTTON", | ||
valid_begin : [ 0, 0 ], |
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.
Please change valid_begin
and valid_end
to run_range: [0, 0]
. Furthermore, RATDB tables should have their keys be in quotation marks.
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.
what do you mean by quotation marks, compared to the ones that have been used e.g. "ENCAPSULATION"
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.
all keys -- e.g. name
, index
, should be quoted. RATDB files should follow standard JSON.
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.
so "name": "ENCAPSUALTION" ? sorry if its something simple but im abit confused looking at other ratdb files i dont see any difference and dont see any json errors my side
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.
Yes. This is a persistent issue with the legacy RATDB files. However, we should not be adding more tables with the incorrect syntax. See https://ratpac.readthedocs.io/en/latest/users_guide/database.html#syntax.
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.
thanks for explaining!
forgot to mention -- would also be nice to have a macro/geo file showing how to use the encapsulation. |
@JamesJieranShen Im glad the misunderstanding has been resolved, apologies on my side I didnt realise some old code was still inside the pmt.info. I will make the changes you have mentioned. |
…oved inital description comment to header file
…oved inital description comment to header file
…akano for private repo
@JamesJieranShen , @tannerbk Requested changes have been made. Functions have been renamed. New material has been removed and existing acrylic used instead. The code was corrected as to not have hard coded From my perspective the code is ready to merge. |
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.
LGTM. Thanks for the hard work!
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.
Just two small comments.
src/geo/src/pmt/PMTFactoryBase.cc
Outdated
} | ||
} | ||
|
||
info << "Creating PMT model" << newline; |
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.
This print statement may spam the log when building a detector with many PMTs.
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.
On button we have an array of 96 PMTs, the statement only shows up once if thats what you mean, or are talking about multiple types of PMTs used? But happy to remove if better.
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.
Ah I missed it was per PMT model, not per PMT. I would still probably avoid adding new statements that print to the log regardless of whether encapsulation is used.
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.
okay thats fine will remove this line.
src/geo/src/pmt/PMTFactoryBase.cc
Outdated
@@ -45,8 +46,55 @@ G4VPhysicalVolume *PMTFactoryBase::ConstructPMTs( | |||
} | |||
G4ThreeVector local_offset = PMTInfoParser::ComputeLocalOffset(mother_name); | |||
|
|||
PMTConstruction *construction = PMTConstruction::NewConstruction(lpmt, mother); | |||
G4LogicalVolume *log_pmt = construction->BuildVolume(volume_name); | |||
PMTEncapsulation *encap_construction = 0; |
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.
I think its slightly preferred that these are initialized with nullptr
rather than 0.
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.
thanks, should I then make similar changes in hemisphereencapsulation.cc when I initialise logical volumes etc?
@tannerbk I have made the changes to PMTFactoryBase.cc |
An encapsulated PMT has been created where the PMT is enclosed within two acrylic domes. This setup is currently used for the BUTTON experiment. The PMT used is r7081pe.
The main file for this request is EncapsulationPMTConstruction.cc which is located within src/geo/src/pmt/
The corresponding header file is located in src/geo/include/RAT/
PMTConstruction.cc and PMTFactoryBase.cc have been modified to take the "encapsulated" construction option.
The CMakefile has been modified to add EncapsulationPMTConstruction.cc
A new PMT called r7081pe_encapsulated has been created in PMT.ratdb.
The "nakano_acrylic" material has been added to material.ratdb and optics.ratdb
The encapsulation setup is customisable, for example we have added additional objects such as optical gel, silica, and pmt wire internally, aswell as addition objects that are part of the external encapsulation.