Fix Xpeak & Ypeak in Penthesilea #777
Conversation
| PATH_OUT = os.path.join(config_tmpdir, 'fake_hdst_nothreshold_barycenter_bias.h5') | ||
|
|
||
| conf = configure('dummy invisible_cities/config/penthesilea.conf'.split()) | ||
| conf.update(dict(files_in = PATH_IN , |
There was a problem hiding this comment.
Maybe you want to change the comma alignment following #776 (comment)
There was a problem hiding this comment.
Is it agreed to change to this alignment style?
There was a problem hiding this comment.
Given that we have
- In favour:
- against:
∅
that is to say, the ratio of (implicit or explicit) favourable to unfavourable opinions is ∞, I'm tempted (in the absence of vehement opposition in the next 24 hours) to conclude that we've 'agreed' to change to this style. Do you have an opinion on it?
This is a choice which has no profound and irreversible consequences. No point in waiting on a decision. Just Do It! (Unless you hate it.)
There was a problem hiding this comment.
Do you have an opinion on it?
I'm in 100% favor of this alignment style. I was just wondering whether it was official or not. I am guessing we will not make the effort of realigning existing code, but rather request new code to follow this rule, right?
There was a problem hiding this comment.
I'm in 100% favor of this alignment style.
Quadruple infinity. I think that settles it.
I was just wondering whether it was official or not.
AFAIAC, it is now.
I am guessing we will not make the effort of realigning existing code, but rather request new code to follow this rule, right?
Exactly.
We can also adapt existing code when we are changing it for some other purpose.
Aretno
left a comment
Aretno
left a comment
There was a problem hiding this comment.
Basic but long needed fix in penthesilea. With this the mean position in the hdst matches that of the kdsts, which was the desired behavior. This will fix a PSF related issue where the user had to recalculate the barycenter in order to use proper positioning. Code is nice and clean and aside form a minor style issue I'd say it's ready to go.
|
Now this is fully ready to be merged. Good job! |
4 participants
This PR addresses issue #730. The computation of the
XpeakandYpeakcolumns used hardwired parameters which includedQthr = 0. This results in a biased computation of those columns. The solution includes the usage of theglobal_reco_paramscity argument (currently used for the kdst side of the pipeline) for the calculation ofXpeakandYpeak.A test together with a new test file has been added. The test file contains a single event with an artificial pmap made to illustrate the problem. The S2 of this pmap contains a single sipm with charge over threshold and a bunch of sipms with charge below threshold (threshold = 5 pes). With the previous configuration, no threshold is applied and therefore all the sipms are taken into account, resulting in a biased result. However, if the threshold is applied (this PR), only 1 sipm survives, and the reconstructed position matches that of the sipm.
Fixes #730.