DUNE / larnd-sim

Simulation framework for a pixelated Liquid Argon TPC
Apache License 2.0
10 stars 26 forks source link

Feature seg hotfix removal #236

Closed YifanC closed 1 month ago

YifanC commented 1 month ago

Remove the hotfix for the segment updates

cuddandr commented 1 month ago

Lines that define TPB/BPG and perform the quench and drift are deleted in this commit. Are they somewhere else in simulate pixels now? The commit diff stops showing the rest of the file and it's easier to just ask.

Relevant lines:

TPB = 256
BPG = max(ceil(all_mod_tracks.shape[0] / TPB),1)
quenching.quench[BPG,TPB](all_mod_tracks, physics.BIRKS)
drifting.drift[BPG,TPB](all_mod_tracks)
YifanC commented 1 month ago

@cuddandr Yes, they are here in the modular loop. The hotfix is to do it again on the entire set. https://github.com/DUNE/larnd-sim/blob/ebaa36c66839d6d6b061572b26ac52ca2637498f/cli/simulate_pixels.py#L673-L697

mjkramer commented 1 month ago

I ran this, and ran the output through flow (feature_backtrack branch), and made the validation plots for both. The plots look good.

However, I had to modify both of the plotters because they assumed that segment_id could be used as an index into segments. That's no longer the case, presumably because of the Active Volume Check (which is now always on when mod2mod_variation is enabled; see comment above). The AVC only filtered out a couple of segments, so is there any reason we can't just leave it disabled for now, given that other downstream code might also treat segment_id as an index?

YifanC commented 1 month ago

@mjkramer I feel strongly that we should avoid using segment_id as index if we can, and should advocate the reconstruction and analysis to not to do that. Another issue is that we should update the boundary in larndsim configuration based on gdml. Directly answering the question why we can't have AVC disabled for mod2mod_variation: in the module loop, the input is segments in a particular module selected with AVC function. The sum of these is as AVC activated for the whole detector (double checked). This led to the hotfix.

YifanC commented 1 month ago

@mjkramer Agreed on loudly informing the changes. I will write a message in the relevant slack channels and mention this in meetings.