WireCell / wire-cell-toolkit

Toolkit for Liquid Argon TPC Simulation and Reconstruction
https://wirecell.github.io/
Other
7 stars 22 forks source link

add protections for xregion configuration #349

Closed wenqiang-gu closed 3 weeks ago

wenqiang-gu commented 3 weeks ago

@brettviren I added two protections related to the new xregion:

First, sometimes "null" values in the faces would introduce a problem because Xregion(null) would be undefined.

Second, AnodePlane requires scalar values (cathode, anode, response) for definition of the BoundingBox of a face's sensitive volume. In such case, we need a backup scalar input, for example:

faces: [
    {
        response: ...,
        anode: ...,
        cathode: {
            x: [...], y:[...], z:[...],  // non-scalar cathode
        },
       cathode_ref: -200*wc.cm, // scalar
    },
    null,
],
brettviren commented 3 weeks ago

Hi @wenqiang-gu

Good catch!

Adding new *_ref configuration attributes makes me a little nervous. What do you think about one or both of these ideas:

The response_x value is used in defining the pimpos origin X value held by the WirePlane. A "non-planar plane" for the response plane would, I think, never make sense given its main purpose is to connect to the plane on which FR paths start. So, I suggest that AnodePlane should throw an exception if it is ever given a non-scalar value for response.

The other two, cathode and anode are used to make a BoundingBox rectangular volume that is meant to characterize the "sensitive volume". Given the name "bounding" I think it is fitting that AnodePlane can use the extreme value for X when it is given a non-scalar cathode or anode "plane". The BB would then still "bound" the entire shaped cathode and/or anode.

wenqiang-gu commented 3 weeks ago

@brettviren I can agree with your point 1 and partially agree with your point 2. See the updated PR.

For point 1, I think it's even better to throw an error when anode is not scalar.

For point 2, setting cathode_x to extreme value (eg, ing/-inf) would be tricky as the DepoTransform or DepoSplat rely on the BoundingBox to check if a depo belongs to a face sensitive volume: https://github.com/WireCell/wire-cell-toolkit/blob/c138b92fecf91c7f573e29226ca8316b06a1fb54/aux/src/DepoTools.cxx#L118 Therefore, it could be easier just to set the cathode_x to be response_x +/- 0.01mm. As the energy depos after Drifter are all relocated at response_x, the slightly enlarged BoundingBox (0.01mm) can guarantee that inside(depo->pos()) returns true for the corresponding face. How do you think?

brettviren commented 3 weeks ago

Sorry, I was not clear.

The "extreme value" does not mean infinity but means the extreme X location of the cathode surface (and etc for anode). This will either be the max() or the min() value of the configured .x array. The max() if the cathode is at larger X values compared to the response plane, min() otherwise. That is, in both cases we wish to find the cathode X location that is the maximum distance from the response plane.

wenqiang-gu commented 3 weeks ago

@brettviren I see, I didn't realize that. How is the updated PR now?

brettviren commented 3 weeks ago

Looks good to me! Thanks for considering the alternatives.