Lhfit highsignal gaussian approximation #9
Conversation
…mation with the original likelihood computation
|
Hello @gabemery! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-01-06 15:46:44 UTC |
There was a problem hiding this comment.
I thought you would leave the non-saturated processing intact and change to gaussian approximation only for saturated events. In fact, in the code I see that you're now computing gaussian, poissonian and full sum for every event, if I'm not mistaken... I would actually start with only doing this for saturated events (nb, the approximation is expected to work better for brighter events/pixels). Then, check the convergence on less bright events in bins of n_photopeaks (for faint events I would expect the approximation to perform poorly).
Also, please make the PEP8 bot happy again :)
lstchain/reco/r0_to_dl1.py
Outdated
| @@ -65,6 +65,7 @@ | |||
| from ..io.io import dl1_params_lstcam_key | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
| logger.setLevel(DEBUG) | |||
There was a problem hiding this comment.
This is a bit dangerous... I would much prefer the logging level to be passed as a parameter, otherwise one needs to modify a source code to change it.
lstchain/reco/reconstructor.py
Outdated
| #May need rework after accelaration addition | ||
| photoelectron_peak = np.arange(n_peaks, dtype=np.int) | ||
| # May need rework after accelaration addition | ||
| photoelectron_peak = np.arange(5*n_peaks, dtype=np.int) # the 5* is more testing only |
There was a problem hiding this comment.
Please expand comment on what's this for
lstchain/reco/reconstructor.py
Outdated
| @@ -574,32 +576,33 @@ def log_pdf(self, charge, t_cm, x_cm, y_cm, width, | |||
| it = it + 1 | |||
| kmin[kmin<0] = 0 | |||
| kmax = np.ceil(kmax) | |||
| if len(kmin)==0 or len(kmax)==0: | |||
| kmin,kmax=0,len(self.log_k) | |||
| mask = (kmax <= len(self.log_k) / 5) # /5 for testing only | |||
There was a problem hiding this comment.
Is this hardcoded constant related to l511? Please avoid such things. If you need, use a local variable
There was a problem hiding this comment.
Yes it is the same 5, I can replace it. But it should disappear before merging as it is only used to have a common range for the previous likelihood computation and the new Gaussian approximation.
|
The approximation cannot be employed event wise. This is due to low signal pixels on the side of the shower. It thus need to have a criteria based on each pixels. Additionally, the likelihood uses a modeled value for the estimation of the Poisson probability of k photo-electron and not the measured value. Here you are a bit confused by the naming of the variables. Both log_poisson and log_gaussian are part of the sum likelihood. the Gaussian approximated log likelihood is log_pdf_HL. So the sum is computed for all pixels (limiting kmax to 5* n_peak, and thus should be accurate as long as this limit is not reached) while the Gaussian approximation is computed only for pixels requiring kmax > n_peak. The next step would be to ignore pixels requiring kmax > n_peak in the estimation of the sum range, and to compute only the Gaussian approximation for such pixels. At the same time, the sum would be only computed for pixels requiring kmax < n_peak. |
…tor used to test the compatibility of the new gaussian approximation with the previous computation
|
Apparently a typo was introduced at some point. As reported by Travis: ____________ ERROR collecting lstchain/reco/tests/test_r0_to_dl1.py ____________
997../../../miniconda/envs/lst-dev/lib/python3.6/site-packages/_pytest/python.py:578: in _importtestmodule
998 mod = import_path(self.fspath, mode=importmode)
999../../../miniconda/envs/lst-dev/lib/python3.6/site-packages/_pytest/pathlib.py:531: in import_path
1000 importlib.import_module(module_name)
1001../../../miniconda/envs/lst-dev/lib/python3.6/importlib/__init__.py:126: in import_module
1002 return _bootstrap._gcd_import(name[level:], package, level)
1003<frozen importlib._bootstrap>:994: in _gcd_import
1004 ???
1005<frozen importlib._bootstrap>:971: in _find_and_load
1006 ???
1007<frozen importlib._bootstrap>:955: in _find_and_load_unlocked
1008 ???
1009<frozen importlib._bootstrap>:665: in _load_unlocked
1010 ???
1011../../../miniconda/envs/lst-dev/lib/python3.6/site-packages/_pytest/assertion/rewrite.py:170: in exec_module
1012 exec(co, module.__dict__)
1013lstchain/reco/tests/test_r0_to_dl1.py:3: in <module>
1014 from lstchain.reco.r0_to_dl1 import rescale_dl1_charge
1015E File "/home/travis/build/cta-sst-1m/cta-lstchain/lstchain/reco/r0_to_dl1.py", line 668
1016E )
1017E ^
1018E SyntaxError: invalid syntaxPlease fix |
|
Ok, I will approve and merge it for the moment, but there's some refactoring required... |
There was a problem hiding this comment.
____________ ERROR collecting lstchain/reco/tests/test_r0_to_dl1.py ____________
996lstchain/reco/tests/test_r0_to_dl1.py:3: in
from travis again:
997 from lstchain.reco.r0_to_dl1 import rescale_dl1_charge
998lstchain/reco/r0_to_dl1.py:68: in <module>
999 logger.setLevel(DEBUG)
1000E NameError: name 'DEBUG' is not defined
Description
A Gaussian approximation of the pixel likelihood is added for high luminosity pixels. High luminosity pixels are identified as pixel requiring to consider a number of possible photo-electron above the parameter n_peak in the likelihood computation.
The goal is to reduce the computation time required for such pixels as the range of Poisson contributing terms for the exact likelihood computation increase with the expected signal. It also limits the number of pre-computed factorials required.
WIP :
Related Issue
Related to #7
How Has This Been Tested?
Currently not tested
Types of changes
Checklist: