Removing hard-coded parameters in irene #686
Conversation
ausonandres
force-pushed
the
irene_studies
branch
2 times, most recently
from
c658576 to
49d213e
Compare
There was a problem hiding this comment.
Generally good, just a few minor points.
I'd prefer there were a way to get the correct binning from the file or database to save errors but the PMap protections should be enough for now though I suspect there might have to be further changes to accommodate the version with PMT rebin pre-selection
You need to rebase too. Maybe do it pre changes to limit clashes.
| from .. types.ic_types import minmax | ||
| from .. database import load_db | ||
|
|
||
| from .. core.system_of_units_c import units |
| import tables as tb | ||
| import numpy as np | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
I'd use the same format as for the other imports so:
instead of import pytest
use from pytest import raises
And the same for any other one that you need
invisible_cities/cities/irene.py
Outdated
| s1_lmin, s1_lmax, s1_tmin, s1_tmax, s1_rebin_stride, s1_stride, thr_csum_s1, | ||
| s2_lmin, s2_lmax, s2_tmin, s2_tmax, s2_rebin_stride, s2_stride, thr_csum_s2, thr_sipm_s2): | ||
| s2_lmin, s2_lmax, s2_tmin, s2_tmax, s2_rebin_stride, s2_stride, thr_csum_s2, thr_sipm_s2, | ||
| pmt_sample_f=25*units.ns, sipm_sample_f=1*units.mus): |
There was a problem hiding this comment.
I'm not sure that I'm happy with the names. The f means frequency, I assume but the value isn't really a frequency but a width. It's not that important maybe but something like 'pmt_samp_wid' might be a bit more correct without making the name too long
| msg = "Shapes don't match!\n" | ||
| msg += "times has length 6\n" | ||
| msg += "pmts has length 6 \n" | ||
| msg += "sipms has length 3\n" |
There was a problem hiding this comment.
I'm not really a fan of this comparison where you're explicitly writing a copy of the error message. Unfortunately, I don't see an obvious fix for it without changing the PMap definition to have the error messages accessible.
| files_in = PATH_IN, | ||
| file_out = PATH_OUT, | ||
| pmt_sample_f = pmt_sample_f, | ||
| sipm_sample_f = sipm_sample_f)) |
| msg += "sipms has length 3\n" | ||
|
|
||
| with pytest.raises(ValueError) as error: | ||
| assert irene(**conf) |
There was a problem hiding this comment.
You don't need the assert here
| time, length, | ||
| stride, rebin_stride, | ||
| Pk, pmt_ids, | ||
| pmt_sample_f =25*units.ns, |
| sipm_wfs = sipm_zs_wf, | ||
| thr_sipm_s2 = thr_sipm_s2, | ||
| pmt_sample_f = pmt_sample_f, | ||
| sipm_sample_f = sipm_sample_f, |
ausonandres
force-pushed
the
irene_studies
branch
from
49d213e to
55f7575
Compare
|
Thank you for the comments. Regarding the first one, the idea was to allow the possibility of specifying pmt and sipm sample widths in the config file, but if you don't want to, standard values will be set at the beginning of the city. The rest of suggestions have been applied. |
andLaing
left a comment
andLaing
left a comment
There was a problem hiding this comment.
Some simple changes that remove hardwired numbers from the PMap construction steps. Makes things more flexible for future updates and is tested.
ausonandres
force-pushed
the
irene_studies
branch
from
fb55d43 to
0ff8023
Compare
#686 [author: ausonandres] The purpose of this PR is to remove some parameters (pmt_sample_frequency=25 ns, sipm_sample_frequency=1 mus, and ratio=40) that were hard-coded in some functions of `peak_functions.py`. Now, they are defined in the input of the city (since they should not change their values), and could be specified in config file. [reviewer: andLaing] Some simple changes that remove hardwired numbers from the PMap construction steps. Makes things more flexible for future updates and is tested.
next-exp#686 [author: ausonandres] The purpose of this PR is to remove some parameters (pmt_sample_frequency=25 ns, sipm_sample_frequency=1 mus, and ratio=40) that were hard-coded in some functions of `peak_functions.py`. Now, they are defined in the input of the city (since they should not change their values), and could be specified in config file. [reviewer: andLaing] Some simple changes that remove hardwired numbers from the PMap construction steps. Makes things more flexible for future updates and is tested.
3 participants
The purpose of this PR is to remove some parameters (pmt_sample_frequency=25 ns, sipm_sample_frequency=1 mus, and ratio=40) that were hard-coded in some functions of
peak_functions.py. Now, they are defined in the input of the city (since they should not change their values), and could be specified in config file.