-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tool to generate a HDF table of sampling interval coefficients #690
base: main
Are you sure you want to change the base?
Conversation
The SamplingIntervalCalculate class to create a sampling interval coefficient table for LST readout system using chip DRS4. | ||
""" | ||
def __init__(self): | ||
self.peak_count = np.zeros([N_PIXELS, N_CAPACITORS_CHANNEL]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zeros
creates a float64
array by default, counts should be integers, right? So specify dtype=np.int64
or another appropriate int dtype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I set the proper data type (uint16 is enough).
""" | ||
def __init__(self): | ||
self.peak_count = np.zeros([N_PIXELS, N_CAPACITORS_CHANNEL]) | ||
self.fc_count = np.zeros([N_PIXELS, N_CAPACITORS_CHANNEL]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
self.charge_reso_array_after_corr ={} | ||
|
||
self.charge_reso_final = np.zeros(N_PIXELS) | ||
self.used_run = np.zeros(N_PIXELS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run is also integer
self.fc_count[np.arange(N_PIXELS), fc % N_CAPACITORS_CHANNEL] += 1 | ||
|
||
|
||
def stuck_single_sampling_interval(self, file_list, gain): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does stuck
mean here? Are you sure that's the right word?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, it is a typo... it should be 'stack'
|
||
if gain_in_file == gain: | ||
if not run_id in self.peak_count_stuck.keys(): | ||
self.peak_count_stuck[run_id] = np.zeros([N_PIXELS, N_CAPACITORS_CHANNEL]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dtype?
if gain_in_file == gain: | ||
if not run_id in self.peak_count_stuck.keys(): | ||
self.peak_count_stuck[run_id] = np.zeros([N_PIXELS, N_CAPACITORS_CHANNEL]) | ||
self.fc_count_stuck[run_id] = np.zeros([N_PIXELS, N_CAPACITORS_CHANNEL]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dtype?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
==========================================
+ Coverage 84.35% 85.05% +0.69%
==========================================
Files 61 61
Lines 5004 5285 +281
==========================================
+ Hits 4221 4495 +274
- Misses 783 790 +7 ☔ View full report in Codecov by Sentry. |
self.sampling_interval_coefficient[run_id] = np.zeros([N_PIXELS, N_CAPACITORS_CHANNEL + N_SAMPLES]) | ||
|
||
for pixel in range(N_PIXELS): | ||
self.sampling_interval_coefficient[run_id][pixel, :N_CAPACITORS_CHANNEL] = \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use implicit line continuations inside parentheses instead of these line breaks with \
.
Also, it looks like this loop is not necessary but could be avoided by using the axis
keyword for the numpy functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the comment. done.
(Concerning the loop, please let me know if there is a more straightforward way...!)
r1_sample_end = r0_r1_calibrator.r1_sample_end.tel[tel_id] | ||
|
||
# Check pulse | ||
pulse_event_flag = np.sum(np.max(waveform[gain], axis=1) > 100) > 1800 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs more context and probably the unnamed constants here should be transformed into either global constants or configurable traitlets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added contexts, and set global constants for them
|
||
|
||
def set_charge_array(self, gain): | ||
self.charge_array_before_corr = np.zeros([N_PIXELS, 60000]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 60000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified related codes. If max_events
is given, this given value will be used to generate a new array.
If not (max_events
is None
by default), just use a large enough number (55000) to cover all events in a single subrun.
(A single subrun usually contains ~53000 events.)
I'm sure it is not the best way... so I'm very happy if you have any good idea...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can actually get the number of events by doing len(source.multi_file)
, I will also add __len__
to the LSTEventSource
, but that will require a new release.
A more flexible solution would be to use a python list and append to it, converting to a 2d numpy array at the end.
self.charge_reso_array_before_corr = np.zeros(N_PIXELS) | ||
|
||
for run_id in self.peak_count_stuck.keys(): | ||
self.charge_array_after_corr[run_id] = np.zeros([N_PIXELS, 60000]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
|
||
__all__ = ['SamplingIntervalCalculate'] | ||
|
||
N_PIXELS = 1855 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can import these from ctapipe_io_lst.constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
self.log.debug('start peak counting') | ||
|
||
for i, event in enumerate(self.eventsource): | ||
if event.index.event_id % 500 == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use tqdm
for a proper progress bar instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I didn't understand tqdm
before:) Now tqdm
is used during even loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this line is not needed anymore
from lstchain.calib.camera.sampling_interval_coefficient_calculate import SamplingIntervalCalculate | ||
from lstchain.paths import run_info_from_filename | ||
|
||
class SamplingIntervalCoefficientHDFWriter(Tool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems just to be three tools in one that don't really share code and even use different traitlets. I think really doing three smaller tools would be better and less confusing (since the help will only show the options for each step, not for all steps).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Actually, I just wanted to gather all of the procedures in a single file. Is it possible to put three smaller tools in a single file and call one of the tools when executing the command...? or one file per one tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do multiple Tools per file and have different entrypoints, but I would just make three files
lstchain_create_sampling_interval_coefficient_file --help | ||
""" | ||
|
||
name = "SamplingIntervalCoefficientHDFWriter" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Seiya,
if I understand correctly you are writing fits files, instead of a HDF5 file.
Would be it possible to write indeed a h5 file? (we decided to prefer this format)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget my comment, I see that the final file is in h5 and also that you put together the gains at the ends.
Perhaps a bit misleading the fact that you have single tool for the three steps
…tion to plot results)
Thank you for the comments, @maxnoe and @FrancaCassol ! I also added a function to produce a result pdf file like below: It would be very appreciated to have a look again. |
Hi @FrancaCassol @maxnoe, |
def plot_results(self, verify_data_path, output_file): | ||
|
||
plt.rcParams['font.size']=15 | ||
camera = CameraGeometry.from_name('LSTCam-002') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an outdated camera (we have now version 4).
Better to use:
from ctapipe_io_lst import load_camera_geometry
camera = load_camera_geometry()
camera = camera.transform_to(EngineeringCameraFrame())
input_fits = traits.Path( | ||
help="Path to the generated peak count fits files", | ||
input_peak_count_fits = traits.Path( | ||
help="Path to the generated peak count fits files by lstchain_create_peak_count_file_for_sampling_interval_calib", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @SeiyaNozaki ,
I do not see this tool in your PR, am I wrong?
self.sampling_interval_coefficient_calculate.convert_to_samp_interval_coefficient() | ||
self.sampling_interval_coefficient_calculate.set_charge_array() | ||
|
||
for i, event in enumerate(tqdm( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for enumerate
, the event already has event.count
This PR is a continuation of #255.
I added a new tool and class to create sampling interval coefficients tables, which is used for
TimeSamplingCorrection
.This tool has three stages:
count pulse peak on each capacitor
stack single fits file, convert into sampling interval coefficients, calculate charge resolution with new coefficients for the verification
merge coefficients of both gains into a single HDF table
TimeSamplingCorrection
already implemented in lstchain.https://github.com/cta-observatory/cta-lstchain/blob/master/lstchain/calib/camera/time_sampling_correction.py#L14