cta-observatory / ctapipe

Low-level data processing pipeline software for CTAO or similar arrays of Imaging Atmospheric Cherenkov Telescopes
https://ctapipe.readthedocs.org
BSD 3-Clause "New" or "Revised" License
63 stars 266 forks source link

psi angle in hillas_parameter functions inconsistent #358

Closed tino-michael closed 4 years ago

tino-michael commented 7 years ago

@kosack and I discovered that there seems to be an inconsistency between the direction of the axis against which the psi angle is defined. Function 1 does not agree with at least function 2 and 4. The psi angle is -- depending on the quadrant the shower is in -- flipped by 180° or flipped in sign. This causes the skewness to flip in sign as well.

@mdpunch, I think you did some major work here. Would you be willing to have a look? I think in the end, we should just remove inferior (because inconsistent or slower) implementations...

mdpunch commented 7 years ago

Fine for me to look at this. I had intended to do some more work anyway, for some suggestions that Karl made about lru-cache, etc. which can simplify the code a lot.

As far as I'm concerned, Psi in function 1 is simply wrong (I suppose depending on context and how psi is used), or at least not general enough. The others are correct (or added/corrected by me).

Psi is defined with respect to [0,0], of course. Asymmetry/Skewness is also defined based on this.

It should be possible based on the returned parameters to have another function (or a wrapper?) recalculate the Psi and Asymmetry/Skewness with respect to any point in the camera (and that would be a good place to also return an Alpha value, for old-time's sake).

What do you think @kosack ?

I agree that in the end we should use just one function, to choose the best one. Function 3 is the worst, and is there mainly for historical interest (translation of my old Whipple code). Function 1 is the next worst, and I think is from some old Magic code. Function 2 is 2nd best for now, but the code looks a bit cleaner than function 4, which is the fastest (pythonized version of function 3).

Since the code just will use one version by default, maybe that's all that's needed for the choice (but not to choose function 1 by default)?

P.S. Unfortunately I can't make it to the next ctapipe meeting, though I did look into it. I will try to connect remotely for parts.

kosack commented 7 years ago

I guess in the end we should just choose one function and remove the others - perhaps version 4 since it's fastest? We can always improve it further in the future.

I think the main issue is just how to define the asymmetry or skewness and phi and their signs: the sign and meaning should be defined in a consistent way and documented.

For example right now phi is in the range 0-360: does it correspond to the direction of the assymmetry as well? Of course it could be modded to 0-180, since an ellipse is symmetric, but we still need a way to then choose the right direction based on the sign of skewness.

I think just having a code snippet example or diagram in the documentation would help: given a hillas ellipse, and two possible directions along the ellipse for the shower, how does one apply skewness to choose the right one.

kosack commented 7 years ago

see also #150

maxnoe commented 4 years ago

We removed all but one hillas parameterization, so this is not an issue anymore