Closed bkatiemills closed 11 years ago
The only thing I have to add is that the primary motivation for this is to reproduce the efficiency measurement from the TIGRESS/GRIFFIN clover acceptance tests. This calls for a 60Co source to be placed 25cm from the front face of a single clover. Greg and Adam requested that if we implement this then we may as well also implement the option of specifying the detector distance.
Perhaps we could keep the existing options /DetSys/det/addGriffinForward and /DetSys/det/addGriffinBack but also add a /DetSys/det/addGriffinCustom where the distance could be specified?
@carlu This seems reasonable. Just so everyone is clear of what the difference is let me mention some obvious things. /DetSys/det/addGriffinForward is the most efficient position with the detectors 11cm from the origin, and the front extension suppression shields pulled back, and the detectors are close-packed. /DetSys/det/addGriffinBack is the best peak-to-total configuration with the detectors moved back to 14.5cm and the front extension suppression shields positioned around the HPGe, fully Compton suppressed.
Either we can change the distance from the origin where the detectors sit, or change the origin where the particles are emitted. The problem we encounter is if we just change the forward_inner_radius we run the possibility of overlapping the suppression shields since they also depend on forward_inner_radius. Maybe we should refactor the code to separate out the forward_inner_radius from the suppression shields. Or we only let the detectors sit between >=11cm to <=14.5cm (just like reality in the array I believe), and if we ever want to simulate, say a source 25cm from the detector face, we move the source 14cm from the origin.
I have no favouritism here, but we should pick a standard.
I think the most usable thing would be to leave the source at the origin, and have one single parameter that just moves the clovers radially away from it by an arbitrary distance.
Then we need to remove the dependence of extension suppression shields from this distance.
Yup we do! That's good factorization - thanks. If this is the only extra thing the radial distance is coupled to, it shouldn't be too hard to separate.
I am working on this at the moment but I just want to be clear about what it is that I am actually trying to do.
Are we trying to remove the dependence of the suppressor position on the forward_inner_radius variable so that we can position the detectors without changing the position of the suppressors?
If this is the case, then am I right to assume that these are the suppressors:
and these are the detectors:
?
forward_inner_radius seems to manifest itself into a few other variables which are spread throughout the code as well. I am assuming that you want a value of 11cm in it's place, is this correct?
Please let me know if I am on the right track or if I am completely wrong.
Thanks.
Hi @tballast.
I think you have correctly identified which volumes belong to the suppressors, and which to the detectors (or HPGe's to be more precise, as the suppressors are also detectors).
You raise an interesting point regarding whether the suppressors should move with the detector or not, my feeling is that either they should move with the HPGe, or alternatively, the suppressors should be omitted from the simulation entirely when the "custom distance" option is called. When a real-world efficiency measurement is done at 25cm the extension suppressors are never installed and the side and back suppressors would not usually be there either.
If we do decide to keep the suppressors in place, and move them with the HPGe, then I think it would be best to do this with them in the "Forward" configuration as this will mean the HPGe's are not shadowed by the suppressors when viewed from the origin/source.
Thoughts?
Hi @tballast and @carlu ,
I would vote for options 2, namely, "the suppressors should be omitted from the simulation entirely when the "custom distance" option is called. When a real-world efficiency measurement is done at 25cm the extension suppressors are never installed and the side and back suppressors would not usually be there either."
Effectively the extension suppressors are fixed to the array, and if I'm not mistaken, very limited in their position. But the side and back are fixed to the detector? Is that right?
The position of the whole clover+suppressor construction, whether or not suppressors are included, and what configuration the suppressors are in are all completely separate questions; the best-factored code should leave all of them answerable independently via exactly three parameters (radial position, suppressors on/off, suppressors forward/backward).
This is called 'loose coupling', and is necessary so that changing one thing doesn't break other parts of the code. Please ensure that this is the case.
Ok, so this would allow the suppressors to be in place for any radial position of the HPGe's, but they would only be allowed in either the forward or back position, so it is up to the user to make sure that they turn the suppressors off if they HPGe's are moved further back. Right?
And just to make sure, is this what it is supposed to look like when you call '/DetSys/det/addGriffinBack 2' ?
That looks right to me. That's the fully suppressed mode ("back").
yes.
or whatever other positions they have available to them, radial position and suppressor configuration are completely independent.
Right! The point is that experiment specific conditions like the suppressors being absent when the detectors are moved should never be hard coded into the simulation; by doing that, we make single-use code that is impossible to repurpose for future projects, and that wastes massive amounts of time and money. Loosely coupled code can be inherited and cherry-picked from for other projects easily, without having to untangle weird, case-specific dependencies between things that are in general unrelated. Great work so far, thanks for posting pics!
That sounds reasonable to me! We'll need to add some suppressor user options via the messengers too. ie. DetectorConstructionMessenger.cc
Hey @evan012345 ,
I have been working on adding in the three parameters to control the suppressor and detector locations as well as refactoring some of the code and I am curious as to whether or not my detector back position is being calculated properly. In the images I posted earlier there was a very small gap visible between the suppressors, but in the latest images that gap has not been visible. The only reason I can find to explain this at the moment is that the previous image I posted may not have had the suppressor shells in place. When I remove them I can see the small gap. Here are the images I have now with and without the suppressor shells in place.
With Shells (no visible gap):
Without Shells (small visible gap):
When I ran the new simulation code that you sent last week I got the following result (with a different macro, suppressors in place):
I just want to be sure that these are being displayed properly and that I do not have something wrong with my simulation code. There doesn't seem to be a problem with the forward position (this was also generated with your new simulation code):
I think everything is OK. Like you said, you may not have had suppressor shells in place before, which would explain the gap. Initially in the suppressed GRIFFIN cc file the foward_inner_radius was set to 25cm, which would imply the last thing the code was used for was efficiency measurements where the suppressors wouldn't be present. Maybe that was the case for you.
As long as Geant4 is spitting out errors indicating overlapping volumes I think we are fine.
Ok, so in order to make this work we are going to want commands to specify how the system is to be configured. Should it be one command that has three arguments? Is it possible to do that? They aren't the same type.
It could be set up like Carl was saying where we add one more command (addGriffinCustom) that would assume the suppressors are off, and you just specify a radial distance.
What would be the best way to do this? It would be ideal if we could specify one command that could accept all three parameters, so that any combination is possible, but I am not sure how to implement that.
Whoa whoa whoa, hold on there - that sounds like a whole lot of infrastructure we don't need. There should just be a variable in the parameters file, call it detectorRadialDistance (or whatever), which I can set to any distance I want, totally independently of anything else. Convolving this with a bunch of other parameters is exactly what we want to avoid - keep it as simple as possible!
Oh ok, if that is not the case then I'm pretty sure we can just use the existing variable once it's decoupled from the others. You will just have to recompile every time you want to change the distance. Should the suppressors be controlled in the same way then? We have two commands right now that assume the suppressors are present, but we want to be able to turn them off as well. These commands wouldn't make a lot of sense if the suppressors were off, which is why I thought that one more command might be reasonable.
We don't want to recompile just to change the distance. We should be able to change this variable in the messengers.
My vote is for a "GriffinCustom" command, I believe @carlu suggested this earlier. Keep the GriffinForward and the GriffinBack commands, where we can change the distance the front face of the Ge detector is from origin (if we want to!), but the default should be 11.0 cm and 14.5 cm respectively. This needs to be decoupled from the extension suppressors, they are fixed to the array structure. Then in the GriffinCustom command we explicitly give it a distance and we don't include any suppressors, this is the "efficiency test" mode.
Take a peak at the DetectorMessenger, you see all the commands in there. The Messenger takes the input from the commands and passes the variable to the DetectorContruction. You have options of what to pass the command, eg. String, Double, Double with a unit (10 keV), ThreeVector, etc. Here is a more complicated one:
addGridDimensionsCmd = new G4UIcmdWith3VectorAndUnit("/DetSys/det/gridDimensions",this); addGridDimensionsCmd->SetGuidance("Set grid dimensions."); addGridDimensionsCmd->SetUnitCategory("Length"); addGridDimensionsCmd->AvailableForStates(G4State_PreInit,G4State_Idle);
A G4UIcmdWith3VectorAndUnit command would look like: /DetSys/det/gridDimensions 10 20 30 cm this would set vector.x() = 10_cm, vector.y() = 20_cm, and vector.z() = 30*cm.
It seems to me that a GriffinCustom command in the DetectorMEssanger seems to me like the best option. This could require up to three parameters (Suppressors On/Off, Distance, Number of Clovers), but I think fixing the suppressors to off, and number of clovers to 1, would be reasonable.
@BillMills Are you suggesting that we should keep the existing DetectorMessenger options (AddGriffin forward/back) and add an independent parameter which controls the distance? So AddGriffinForward would no longer imply 11cm, it would only imply that the shield extensions were pulled back? Or should AddGriffinForward imply 11cm but it can be overridden by another parameter? If so, we would need another option to control if the shields were present or not. Then we could do:
/DetSys/det/ShieldsPresent 0 /DetSys/det/SetGriffinDistance 25 /DetSys/det/addGriffinForward 1
to create a single clover, no shields, which can be used for a relative efficiency measurement.
This would seem to be workable to me, but I think I favour:
/DetSys/det/addGriffinForward n (Add n detectors, with shields, front shields retracted, at 11cm) /DetSys/det/addGriffinBack n (n detectors with shields, front shields forward, at 14.5cm) /DetSys/det/addGriffinCustom x (one detector, no shields, at x cm)
These are good points all, but I can see we're thinking along different lines here; you're trying to answer the question, 'how do we accommodate GRIFFIN?', but I think the question is, 'how do we accommodate any GRIFFIN-like array?' - by responding to my question, we code with an eye to building reusable code, not just for GRIFFIN, but for every project.
You are absolutely right that we shouldn't be recompiling to change distances, a macro command is the way to go. But we also shouldn't be hard-coding the characteristics of any one array into our code when we don't have to - that just leads to endless reinvention of the wheel down the line. The three independent options @carlu identified is the best engineering choice, but I would also make sure it's a simple option to decide how many clovers are deployed in this way, from 1 to all of them - but perhaps this can be a next step. For now, lets just worry about decoupling shields, shield configuration, and detector radial distance. To accommodate @carlu 's preferred solution, consider another set of commands that set the independent parameters to what they need to be for typical GRIFFIN configurations; that way, we have a generic, reusable layer, and an independent GRIFFIN-specific layer for ease of use; for example, a command like
/DetSys/det/addGriffinForward n
which sets
/DetSys/det/ShieldsPresent 1 /DetSys/det/SetGriffinDistance 11 /DetSys/det/addGriffinForward 1
for n detectors - full independent control of parameters and ease of use for GRIFFIN.
Hey @carlu , I would love to send you the code I have so far for you to take a look at, but I don't have your email address. I don't want to commit it to my repo yet because it doesn't quite work, so I would rather just send you the files.
Hey Tyler. It's cu@triumf.ca. On 9 Oct 2013 08:48, "Tyler Ballast" notifications@github.com wrote:
Hey @carlu https://github.com/carlu , I would love to send you the code I have so far for you to take a look at, but I don't have your email address. I don't want to commit it to my repo yet because it doesn't quite work, so I would rather just send you the files.
— Reply to this email directly or view it on GitHubhttps://github.com/GRIFFINCollaboration/detectorSimulations/issues/19#issuecomment-25982306 .
@tballast @carlu, if you have some non-functioning code, commit it to a branch! I'll show you how to do this today - per usual, emailing code around is never what we want to do.
Cool. Let me know when you've got the branch up @tballast and I'll check it out.
Hey @carlu , The code id located here : https://github.com/tballast/detectorSimulations/tree/radialDistanceDev
At the moment it is possible to turn the suppressors on and off, but the positioning is still pretty wonky. Take a look in the gui.mac file and you will see the three commands that I am using to actually construct the detector. When you change the radial distance at the moment you will see that the suppressors do not move, and the entire detector moves, but not by the same amount. The current goal is to be able to shift the detector as well as the side suppressors all as one unit. I was under the impression that the suppressors were supposed to be stationary, but I was wrong. My next goal is going to be to isolate the side suppressors from the front suppressors, so that changing the distance will only affect the side suppressors. If you feel like getting further into this it may be worth meeting in person to discuss it.
Hey @evan012345 , I think that I've figured out how to isolate all of these parameters, and I'm just curious about how to properly assemble them. We currently have multiple assemblies for different parts of the detector ( ie. germaniumAssembly, suppressorShellAssembly, etc), and I was wondering if you knew of a way to stack multiple assemblies. That way we could have the germanium cores in their own assembly, but these could be included in a master assembly that consisted of the whole detector without the suppressors. This would allow us to position the whole detector very easily, without having to make sure that each of the individual assemblies was shifted by a specific amount. So far I haven't been able to find a way to do this, so I was wondering if you knew of anything like it?
Hey @tballast, I've just checked out the branch you listed there but it doesn't compile for me. I'm getting lots of errors of the sort:
DetectionSystemGriffinSuppressed.cc:252:9: error: ‘class DetectionSystemGriffin’ has no member named ‘suppressor_shells_include_flag’
Do I have an outdated version of the NDA files? I'm using version 12 from the svn repository. I can debug whatever's wrong if necessary but I just want to make sure we have the same versions.
@carlu whooooops my bad, I haven't reposted the suppressed files yet, too many balls in the air right now - posting immediately....
@carlu rev 13 checked in, give that a try
@carlu , I don't think that the posted versions will work properly. I have had to make some changes that I am experimenting with at the moment, and these haven't been posted. I emailed you the copies that I am using at the moment (sorry for emailing, but they aren't completely stable versions). You will probably need to use those.
@tballast , I've never heard of "stacking" Geant4 assemblies, or putting one assembly inside another. It's worth a google though. However, the reason we had multiple assemblies before was to keep sensitive volume separate from insenstive ones. We no longer have that problem. If you wanted to place everything into one master assembly (except the suppressors) and then place that master once, and then deal with the extension suppressors, nothing stops you from doing this. Good idea! Just make sure when you place volumes into the assembly that you keep the order, ie crystal one is passed to the assembly first, and so on.
Ok, so I think that I have managed to get this to a functional state. There is room for improvement but this will suffice. I have attached screenshots of detectors 4 through 11, in a ring.
Standard Position, suppressors ON and forward:
Standard Position, suppressors ON and back:
Standard Position, suppressors OFF:
Radially shifted by increments of 3 cm, suppressors OFF:
Radially shifted by increments of 3 cm, suppressors ON:
You will notice that when the suppressors are on and the detector is shifted radially, the extension suppressors stay in place, while the back and side suppressors shift along with the detector. At the moment I don't have a way of shifting the extension suppressors as well, but I have been told that this is something that would never be done in practice. I know that this is not as modular as it could be, but that is one improvement that still needs to be implemented. Prior to this modification of the code, all of the suppressor shells (extension, side, and back) were grouped together in ONE block, while each group of suppressors themselves were grouped separately. This is no longer a logical way to group things. The suppressors as well as the shells should be in the same block, so that they do not need to be placed separately. This will make it easier to place them, and it will also allow for greater flexibility when placing. I will continue to try and solve this, but I will admit that I am having some difficulties with it, and would appreciate if someone could take a look as well and maybe give me some advice. This code is in my repo at the moment, if anyone would like to take a look and make sure I haven't made silly changes that should not be pushed to master. I have modified the suppressed files as well, so just let me know and I can send you them.
The new commands that can be used to control the suppressors and radial distance are:
/DetSys/det/SetRadialDistance 22.2 cm /DetSys/det/ShieldsPresent int ( 0 = off, 1 = on ) /DetSys/det/SetExtensionSuppressorLocation int ( 0 = suppressors back, 1 = suppressors forward )
/DetSys/det/addGriffinCustom int (this works the same way that addGriffinForward and addGriffinBack do) /DetSys/det/addGriffinCustomDetector int (this works the same way that addGriffinForwardDetector and addGriffinBackDetector do)
Let me know if there are changes that need to be made. The addGriffinForward/Back command still work as expected, as well as the new commands that Evan recently included. They are not affected by the ShieldsPresent, SetRadialDistance and SetExtensionSuppressorLocation commands.
Looks good Tyler! So you mean in your last paragraph there, that ShieldsPresent, SetRadialDistance and SetExtensionSuppressorLocation are all overridden by the others? This is a reasonable way to go about this, should be fine.
@evan012345, if you have time maybe you can drop by @tballast and my office sometime today and we can have a look at this shells issue together.
The initial Griffin commands (GriffinForward, GriffinBack) have no dependence on ShieldsPresent, SetRadialDistance, and SetExtensionSuppressorLocation. I have essentially hard coded them to give you the expected result (this is what was done before as well). Its very simple to change this, but it seemed logical to do it this way. If you want to change any of the parameters, you can use addGriffinCustom.
Okay but what happens if I do: addGrfiffinForwardDetector 4 SetRadialDistance 123 cm
or the same thing in the opposite order?
If you execute them in that order then you will get one detector (number 4) placed at 11 cm and then you will set the distance variable to 123 cm.
If you execute them in the opposite order then you will set the distance variable to 123, and then you will place one detector (number 4) at a distance of 11cm.
If you were to execute the following:
addGriffinCustomDetector 4 SetRadialDistance 123 cm
You would place one detector (again, number 4) at a distance of 11cm (I set this as the default distance).
If you executed it in the reverse order then you would get the fourth detector placed at a distance of 123cm.
If you wish to adjust the parameters you must do so before you call addGriffinCustom.
So AddGriffinForward is never affected by SetRadialDistance, and addGriffinCustom is only affected if SetRadialDistance is set first - sure, that sounds just fine. Please write a wiki page describing the behaviour of any commands you've introduced, and how they interact and compare to related commands such as these.
Issue resolved at commit 81dff87 - good work everyone.
We need to refactor the parameterization of HPGe placement so that the target to detector distance can be controlled by a single parameter, possibly forward_inner_radius. @evan012345 , please advise, @carlu please add any details or clarification necessary.