New context #72
New context #72
Conversation
|
The last commit updates the S1 LCE map handling. The map is now calculated from the S1 pattern map like it is done in WFSim: https://github.com/XENONnT/WFSim/blob/0c7dee5157ce23461c6b0536c932ebd54cd5fecc/wfsim/load_resource.py#L246-L250 |
|
The tests ran without problem. I think the PR is ready for review. |
There was a problem hiding this comment.
Hey @HenningSE, finally here :) I tested the context and saw how the different configs are tracked and everything works perfectly as far as I can tell. This is clean work, as the rest of fuse!
I am now simply going through the code in detail (also checking back into WFSim to try to remember how some corrections were added) and have minor comments, see below. In addition, could you please remind me what we decided with the s1_lce_correction_map option from WFSim? See description here. Sorry if I missed something we have already discussed, it's been a while. I'll finish reading tomorrow morning.
| self.gains = self.gains = pmt_gains(self.gain_model_mc, | ||
| digitizer_voltage_range=self.digitizer_voltage_range, | ||
| digitizer_bits=self.digitizer_bits, | ||
| pmt_circuit_load_resistor=self.pmt_circuit_load_resistor | ||
| ) | ||
|
|
||
| self.pmt_mask = np.array(self.gains) > 0 # Converted from to pe (from cmt by default) |
There was a problem hiding this comment.
If I do not miss something obvious, I think that this part of the code and the commented function below could be just erased, together with the gains URL config for this plugin. I think the gains were meant to mask the data-driven maps to keep backwards compatibility with ancient data-driven configs in WFSim.
There was a problem hiding this comment.
Hi @ramirezdiego, the pmt_mask is used when building the s2_pattern_map. It is not very obvious and hidden in the URLConfig:
fuse/fuse/plugins/detector_physics/electron_extraction.py
Lines 129 to 137 in a80e0e1
To clean up the code I removed the lines below that were not used.
There was a problem hiding this comment.
Indeed it is used to build the map, like in the analogue S1 plugin. My point here is that, if we remove the ancient code in which s2_pattern_map is propagated for the extraction efficiency correction, this config is no longer needed, is it? We could then remove the gains and pattern map dependences here, but maybe I missed something obvious reading through this class.
There was a problem hiding this comment.
You are totally right, s2_pattern_map is not used in the plugin so I removed it. Thanks for spotting this.
There was a problem hiding this comment.
Great, then we also remove
https://github.com/XENONnT/fuse/blob/new_context/fuse/plugins/detector_physics/electron_extraction.py#L151C1-L157C50
and
https://github.com/XENONnT/fuse/blob/607eb5b44d0aa2878ee2896227b4fd0d86be5a49/fuse/plugins/detector_physics/electron_extraction.py#L105C1-L109C6
and pmt_gains from
right? And that would be all 😄
There was a problem hiding this comment.
Okay, done in ff14f2b. Also removed the arguments involved in the gain calculation, as pointed out by you.
Clarifying the |
|
Ahh thanks for clarifying! I dropped most of the backwards compatibility stuff to clean up the code and have a good implementation of the latest WFSim version. If someone needs these old functions we can add them later in dedicated plugins or so. |
This PR will add a proper simulation context to fuse.