NSB tunning : pre-generated NSB only waveforms and a new class #1274
Conversation
…possibility to pre-generate NSB only waveforms to randomly apply to events.
|
Hi Grabiel, all, Won't this negatively affect image-based methods that do not use cleaning? |
|
Hi @vuillaut. You mean that using correlated nsb injection could lead to issues? Or are you speaking of something else? |
Hi. |
|
It also concerned me, but with the current implementation I think it should not be a massive issue because:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1274 +/- ##
==========================================
+ Coverage 73.35% 73.44% +0.08%
==========================================
Files 134 134
Lines 14117 14161 +44
==========================================
+ Hits 10356 10400 +44
Misses 3761 3761 ☔ View full report in Codecov by Sentry. |
| @@ -130,6 +130,9 @@ def test_r0_to_dl1_lhfit_mc(tmp_path, mc_gamma_testfile): | |||
| config['lh_fit_config']["use_interleaved"] = True | |||
| config['waveform_nsb_tuning']['nsb_tuning'] = True | |||
| r0_to_dl1(mc_gamma_testfile, custom_config=config, output_filename=tmp_path / "tmp.h5") | |||
| os.remove(tmp_path / "tmp.h5") | |||
| config['waveform_nsb_tuning']['pre_computed_multiplicity'] = 0 | |||
There was a problem hiding this comment.
wasn't it already 'pre_computed_multiplicity'=0 in the first place?
There was a problem hiding this comment.
It defaults to 10 from line 432 in r0_to_dl1.py (and in the class, but this is not used) and is not defined in the standard config file.
I should probably add it there too as being 10 for consistency. (Or make it None in the default config file and change the code to support it)
And in this test it may be clearer if I explicitly select its value as 10 to avoid confusion.
lstchain/image/modifier.py
Outdated
|
|
||
| """ | ||
| log.info(f"Pre-generating nsb waveforms for nsb tuning and telescope id {tel_id}.") | ||
| n = 25 |
There was a problem hiding this comment.
this n = 25 is defined several times, does it make sense to define it once at the beginning of the file?
There was a problem hiding this comment.
It does, I will change it.
…efinition done twice in WaveformNsbTunner to init. Clarify parameter naming and add comment.
In order to implement the possibility to pre-generate nsb waveforms to add nsb to events instead of generating new nsb waveforms on the fly, a new class was added.
The class
WaveformNsbTunnertake a new config file entrypre_computed_multiplicitywhich can be set to 0 to generate nsb on the fly. Or to any int to pre-generatepre_computed_multiplicityX number of pixels X number of gains waveforms.Closes #1273
To do before removing draft :
Some processing duration to illustrate the gain on a single (proton) MC file :
+50% NSB, no pre-generation : 1min 39s
+50% NSB, 10X pre-generation : 1min 28s
+1000% NSB, no pre-generation : 3min 18s
+1000% NSB, 10X pre-generation : 1min 31s
Results of the tuning are compatible between using pre-generated waveforms or on the fly generation. Results are also the same as the previous implementation. See the signal less pixel charges for a data runs vs MC without and with tuning.
Unrelated to this PR , but the injection of 55% nominal NSB here is a bit higher than automatically extracted using calculate_required_additional_nsb (~40%). This will need to be investigated independently.