DUNE / larnd-sim

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

NDLAr Specific Additions (Included in latest `MicroProd`) #252

Closed alexbooth92 closed 2 months ago

alexbooth92 commented 2 months ago

A couple of commits that featured in the latest NDLAr MicroProds introducing two things. Want to bring these into develop.

YifanC commented 2 months ago

Hi @alexbooth92 @mjkramer, Yes, I think Alex caught a bug that we could introduce multiple triggers in the batching. I added a version of correction in the MR6 production version, see here. Please let me know if I missed anything. I also thought Kazu would start to play with nd label making so I also added the light trigger mode in the ndlar config and it's currently in develop. Hope it's not causing terrible confusions.

alexbooth92 commented 2 months ago

Thanks for the explanation @YifanC - I must say that I was a little confused when I was sorting out the merge conflicts. I haven't tested your fix but from a brief look I think it does the trick. I have remove any trace of my version of the fix and the "duplicate" LIGHT_TRIGGER_MODE line in the ndlar config. The other stuff related to light trigger mode needs to stay otherwise you can't run with the light sim "turned off".

alexbooth92 commented 2 months ago

If I have understood all of the above correctly, @mjkramer / @YifanC please merge this if things look OK. I can merge after 24 hours otherwise if no further comments.

YifanC commented 2 months ago

@alexbooth92 The current develop support light_simulated=False if you set the light_trigger_mode, which I think you would need for the trigger packet from the larpix simulation. Could you check again? The two remaining changes:

  1. Disable light triggers from the simulated light data when the light simulation is off. I think either way is fine. Happy to have it merged though.
  2. The addition after "except keyerror", it seems to be a copy of these two lines. Would it make more sense to set LIGHT_TRIG_MODE = 1 directly if it gets there? Otherwise I think it will makes no difference?
alexbooth92 commented 2 months ago

@alexbooth92 The current develop support light_simulated=False if you set the light_trigger_mode, which I think you would need for the trigger packet from the larpix simulation. Could you check again? The two remaining changes:

  1. Disable light triggers from the simulated light data when the light simulation is off. I think either way is fine. Happy to have it merged though.
  2. The addition after "except keyerror", it seems to be a copy of these two lines. Would it make more sense to set LIGHT_TRIG_MODE = 1 directly if it gets there? Otherwise I think it will makes no difference?

Thank you for looking! You absolutely need the except key error as I have it to be able to run with light_simulated=False because if not you have to provide all of the light inputs anyway in the config that you pass to simulate_pixels which doesn't make any sense if you are just running a charge simulation. Sorry I should have made that clearer.

So yes, in develop it might support light_simulated=False but you have to provide all the light configs and waste time loading it all. With this set up you don't have to :)