search

DUNE / larnd-sim

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

Fixing some naming/labeling inconsistencies #152

Closed sam-fogarty closed 1 year ago

sam-fogarty commented 1 year ago

I noticed some inconsistencies in the naming/labeling of some variables like eventID. Some of these inconsistencies have led to an error or two when running a fresh clone of the develop branch. The changes I have made are small and are the following:

  1. In dumpTree.py, changed some parameter names, like eventID, vertexID, parentID, etc, to match the new definitions described in 2x2_sim (https://github.com/DUNE/2x2_sim/wiki/File-data-definitions). So eventID->event_id, vertexID->vertex_id, etc.

  2. I saw that despite defining the event_separator in simulation_properties, simulate_pixels.py uses a hardcoded event_id. I just changed those instances to sim.EVENT_SEPARATOR, like elsewhere in the code

  3. In simulation_properties/singles_sim.yaml, add missing tracks_dset_name and change event_separator to event_id, like in simulation_properties/2x2_NuMI_sim.yaml

  4. In consts/sim.py, change default EVENT_SEPARATOR and TRACKS_DSET_NAME to event_id and segments respectively

dumpTree.py and simulate_pixels.py run without errors on a miniRun3 spill file with these changes.

sam-fogarty commented 1 year ago

Thanks for the feedback @krwood. I'll revert those changes you referred to in your last comment, I didn't realize these were unnecessary changes. And yes I agree that it's not ideal to that two different versions of the conversion script. Maybe this script could live in the larnd-sim repo, and 2x2_sim could use it since larnd-sim will be installed along with 2x2_sim? But maybe there's good reason to keep a version of it in 2x2_sim, so @mjkramer might have more insight.

sam-fogarty commented 1 year ago

@krwood I made the change of reverting sim.EVENT_SEPARATOR to event_id in simulate_pixels.py. I did keep the sim.EVENT_SEPARATOR in one instance of vertices, since I think this makes sense given that instances of tracks uses sim.EVENT_SEPARATOR (let me know though if you also want this one changed). Though I'm wondering what the use of sim.EVENT_SEPARATOR really is. Is this something we actually need?

krwood commented 1 year ago

That is a very fair point. We introduced sim.EVENT_SEPARATOR when we were using event_id to specify distinct interactions and spill_id to specify which interactions are grouped together into a spill. In order to be sure that all activity from a spill was being simulating in the same batch, the spill simulation started to use the spill_id to separate the batches of segments. We made it configurable to retain backwards compatibility with, e.g., cosmic simulations. In the 2x2-NuMI simulation, we have since decided to repurpose event_id to mean what spill_id used to mean, and vertex_id to specify distinct interactions (what event_id used to mean). And a consequence of that is that all users (AFAIK) are using event_id as the sim.EVENT_SEPARATOR, making it rather pointless. Given it's already implemented, I'm nonetheless leaning towards keeping the feature in in case we want to utilize it in the future. Although I don't have a very strong preference.

sam-fogarty commented 1 year ago

Yeah I agree we might as well keep it, in case we want to use it later.