Add tools for PSF creation. #668
Conversation
There was a problem hiding this comment.
As for the other PR, this is not a complete review, I'm just trying to provide an outsider's (pedantic) view of this.
Also for this PR, review the naming of variables. In this one, you mix lower_case with camelCase and CamelCase. The last one is particularly problematic because it clashes with the actual naming convention we use: lower_case for variables, CamelCase for classes/types.
Overall there is only one "major" (for a very loose definition of major) concern and one function I don't quite understand. The rest are minor details.
|
Hereby I declare that if IC moves to Haskell it WON’T be my fault....
|
:-) Haskell for IC (or HEP) would be a disaster. There are too many complexities, too much experimentation in the language (no serious work is done without using a number of language extensions which range from "why the hell isn't this in the standard" to "here be dragons"), the error messages are (all too often) impenetrable. And the build system situation is deep tragedy. You should not ask people whose primary interest is not programming, to do serious work in Haskell. OTOH, within Haskell there are many things which would be absolutely marvellous for HEP. If you could get those without the complications and distractions, then it would be a huge boon. This has been done for front-end web development with Elm. Elm is a simplified Haskell derivative that compiles to JavaScript and has outstandingly clear and helpful error messages and a build story that won't drive you insane. It gives web developers many of the advantages of Haskell without any of the problems. A language like that, but with a different set of power-vs-simplicity tradeoffs, a set more appropriate for HEP, could do wonders. I'm not aware of a viable candidate, so I'm afraid that (amongst many other disadvantages) we'll have to keep using Python's shitty type annotations instead of a robust, informative and expressive static type system built into the language; and continue to distinguish pure from impure code without the help of the type system and thus have to rely on @gonzaponte's review comments to remind us that
(We'll also have to continue to compose our components (functions!) using a very clunky syntax. I tried to mitigate this by giving you dataflow, but it's still extremely sad compared to how it could look if we had Haskell syntax for this purpose, but that's another story.) |
Aretno
force-pushed
the
PSF_creator
branch
2 times, most recently
from
e90cb12 to
430491c
Compare
|
Notebook explaining the contents of the PR: https://github.com/Aretno/Notebooks/blob/master/Tools%20for%20PSF%20creation%20auxiliar%20Notebook.ipynb Also, I modified the code to make it a bit more compact but no big changes were made. |
| def psf_writer(hdf5_file, **kwargs): | ||
| psf_table = make_table(hdf5_file, | ||
| group = "PSF", | ||
| name = "PSFs", |
There was a problem hiding this comment.
Maybe a better name for either the group or the node? /PSF/PSFs seems a bit redundant.
There was a problem hiding this comment.
Any suggestions? I don't really care much.
There was a problem hiding this comment.
I don't have much better ideas...
There was a problem hiding this comment.
It seems to be irrelevant as the file contains only this info and it is loaded through pd.read_hdf
| def psf_writer(hdf5_file, **kwargs): | ||
| psf_table = make_table(hdf5_file, | ||
| group = "PSF", | ||
| name = "PSFs", |
There was a problem hiding this comment.
It seems to be irrelevant as the file contains only this info and it is loaded through pd.read_hdf
Class model for PSF dataframe
There was a problem hiding this comment.
This PR adds the relevant functions for PSFs, an input needed for the future city Beersheba (#666). The code adds the functions to create the PSF and the functions for IO operations. All of that is nicely implemented, well tested and with some nice protections against bad inputs.
The author has also provided a notebook with a demonstration of the code:
https://github.com/Aretno/Notebooks/blob/master/Tools%20for%20PSF%20creation%20auxiliar%20Notebook.ipynb
Great work!
next-exp#668 [author: Aretno] Functions and tests for PSF creation starting from penthesilea Kr hits (1 hit = 1 sipm). For the PSF, empty bins surrounding the charged SiPMs are added (as zero's are also relevant). The PSF is calculated using the relative position between SiPMs and barycenter. In the case of the Z dimension, the relative positions is between the slice position and the barycenter in the Z dimension. [reviewer: gonzaponte] This PR adds the relevant functions for PSFs, an input needed for the future city Beersheba (next-exp#666). The code adds the functions to create the PSF and the functions for IO operations. All of that is nicely implemented, well tested and with some nice protections against bad inputs. The author has also provided a notebook with a demonstration of the code: https://github.com/Aretno/Notebooks/blob/master/Tools%20for%20PSF%20creation%20auxiliar%20Notebook.ipynb Great work!
Functions and tests for PSF creation starting from penthesilea Kr hits (1 hit = 1 sipm).
For the PSF, empty bins surrounding the charged SiPMs are added (as zero's are also relevant). The PSF is calculated using the relative position between SiPMs and barycenter. In the case of the Z dimension, the relative positions is between the slice position and the barycenter in the Z dimension.