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

YZ Dependent Shape and Scale Simulation #270

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jzennamo
Copy link

This pull request enables the simulation of different signal response functions and gain scales in different locations of the ICARUS YZ plane. Currently there are 15 different regions on each plane of each TPC which creates 360 subvolumes that are simulated.

This starts by using a new DepoSetFilter called DepoSetFilterYZ.

This PR requires:

  1. Files that are stored in icarus_data in a feature branch called feature/jzennamo_addingYZresponsefunctions
  2. An update to the SimChannel creators in LArWirecell: LArWireCell Update to enable multiple signal response simulation across the YZ Plane  LArSoft/larwirecell#44
  3. An update to the ICARUSCode fcl files which drive this: YZ 2D Simulation and Separate Electron Lifetime Simulation SBNSoftware/icaruscode#611

@@ -64,7 +64,7 @@ function(params, tools)
},
}, nin=1,nout=1,uses=[tools.random, anode] + pir_trio),

make_depotransform :: function(name, anode, pirs) g.pnode({
make_depotransform :: function(name, anode, plane, pirs) g.pnode({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can not change this function signature as it will break existing callers. Given this is such a special purpose I suggest leaving the Jsonnet untouched and simply making your object by hand in the icarus config.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about I make it a separate function? It'd be a copy that includes "plane" with a different name. My hope this that this would be usable for SBND (and DUNE) places where there are wrapped wires will have to deal with effects like this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be legitimate to extend the function signature like:

function(name, anode, pirs, planes=[0,1,2])

In this way, old code which does not include planes will get [0,1,2] which is coincident with the default set in the C++.

Comment on lines +136 to +137
} for plane in [0,1,2]], $.fields[r])
for r in std.range(0,14) ],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can not change the return value nor introduce icarus specific quantities here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do you recommend addressing this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see already a

cfg/pgrapher/experiment/icarus/icarus_tools.jsonnet

which even has a pirs definition. So, I suggest this would be a good place to further modify.

But, some care is needed to not necessarily step on that if other ICARUS jobs rely on the current, uniform set of pirs.

Comment on lines +97 to +98
double yoffset = 180*units::cm;
double zoffset = 900*units::cm;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can not use magic numbers in the C++. These should be made configurable parameters.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved.

Comment on lines +109 to +113
if(depo_bin_y > 31)
depo_bin_y = 31;

if(depo_bin_z > 180)
depo_bin_z = 180;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No magic numbers

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved.

gen/src/DepoTransform.cxx Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants