Add variables to set region of photon emission #143
Conversation
User can set values of these new variables directly, in which way the region of photon emissions can be set, based on the interest of the user.
|
A new Pull Request was created by @szhang17phys (Shuaixiang) for develop. It involves the following packages: larsim @LArSoft/level-2-managers, @LArSoft/level-1-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 (skipped because base branch is not develop) |
|
code-checks |
|
The code-checks are being triggered in jenkins. |
|
-code-checks Then commit the changes and push them to your PR branch. |
|
@szhang17phys please run clang format as noted |
|
Pull request #143 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 Then commit the changes and push them to your PR branch. |
|
Pull request #143 was updated. @LArSoft/level-2-managers, @LArSoft/level-1-managers can you please check and sign again. |
|
+code-checks |
There was a problem hiding this comment.
@szhang17phys, thank you for the PR. Please see the comments below.
| , fBminX{pset.get<double>("BminX")} | ||
| , fBmaxX{pset.get<double>("BmaxX")} | ||
| , fBminY{pset.get<double>("BminY")} | ||
| , fBmaxY{pset.get<double>("BmaxY")} | ||
| , fBminZ{pset.get<double>("BminZ")} | ||
| , fBmaxZ{pset.get<double>("BmaxZ")} | ||
|
|
There was a problem hiding this comment.
Two things:
- I'd rename the configuration parameters (e.g.)
Xmininstead ofBminX. - Also, this change would require all users to specify
"Bmin*"and"Bmax*"parameters in their configurations, and that is not always desired. It would be better to make it an option--i.e. if the parameterYminis provided, then it should override the value offYmin.
| fXmin = fBminX; | ||
| fXmax = fBmaxX; | ||
| fYmin = fBminY; | ||
| fYmax = fBmaxY; | ||
| fZmin = fBminZ; | ||
| fZmax = fBmaxZ; |
There was a problem hiding this comment.
See above comment. These assignments should only be made when the configuration parameter is provided. Otherwise, the values of fXmin, fXmax, etc. should be set to CryoBounds.MinX(), etc.
|
Thanks for above nice comments! I will modify accordingly! |
|
Pull request #143 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. |
The function of UserD is to determine whether applying user-defined region or not. The default value of UserD is false. Also, default values of all user-defined boundary variables, ex: BminX, BmaxX, are all set as 0; Only when UserD is set as true, can users set the region of their own interests. When UserD is not mentioned in fhicl script, the default region is just the boundary of whole cryostat.
|
Pull request #143 was updated. @LArSoft/level-2-managers, @LArSoft/level-1-managers can you please check and sign again. |
|
The code-checks are being triggered in jenkins. |
|
-code-checks Then commit the changes and push them to your PR branch. |
|
@szhang17phys please run the code formatting #143 (comment) |
There was a problem hiding this comment.
Thanks for the updates @szhang17phys --we're getting closer. I was thinking of something like this:
PhotonGen::PhotonGen(fhicl::ParameterSet const& pset)
: ...
{
if (auto boundaries = pset.get_if_present<fhicl::ParameterSet>("boundaries")) {
fXmin = boundaries->get<double>("Xmin");
fXmax = boundaries->get<double>("Xmax");
fYmin = boundaries->get<double>("Ymin");
fYmax = boundaries->get<double>("Ymax");
fZmin = boundaries->get<double>("Zmin");
fZmax = boundaries->get<double>("Zmax");
}
else {
art::ServiceHandle<geo::Geometry const> geo;
auto const CryoBounds = geo->Cryostat().Boundaries();
fXmin = CryoBounds.MinX();
fXmax = CryoBounds.MaxX();
fYmin = CryoBounds.MinY();
fYmax = CryoBounds.MaxY();
fZmin = CryoBounds.MinZ();
fZmax = CryoBounds.MaxZ();
}
}where boundaries would be a nested table in the configuration:
photonGen: {
module_type: PhotonGen
boundaries: {
Xmin: ...
}
}If the boundaries parameter is in the configuration, then the module will take the parameters from the configuration; otherwise they will be provided by the geometry system. This involves the following changes:
- Removing the
fUserD,fBmin*, andfBmax*variables, and - Moving the setting of the
fXmin(etc.) variables to the constructor.
Let me know if you have any questions.
|
Hello! Kyle. I think I got your idea: You do not want to introduce too many variables. fXmin* will replace my fBminX* in PhotonGen::PhotonGen(fhicl::ParameterSet const& pset). If fXmin, fXmax, ... are not pointed to certain values by user, they will get values from cryoBounds automatically. I agree that your proposal is more neat! I will modify again recently! |
|
Pull request #143 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 Then commit the changes and push them to your PR branch. |
User can invoke the new variables directly at the fhicl file. Then region of photon emission can be determined based on the interest of the user, which is more flexible than the initial module, where photon emission vertices can only be spread in the whole space.