Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename confusing Hillas angular parameters #2272

Closed
kosack opened this issue Mar 1, 2023 · 5 comments
Closed

Rename confusing Hillas angular parameters #2272

kosack opened this issue Mar 1, 2023 · 5 comments

Comments

@kosack
Copy link
Contributor

kosack commented Mar 1, 2023

Right now we have hillas.phi and hillas.psi, which are somewhat easy to confuse without reading the descriptions. It might be better to rename them to wordier, but more descriptive versions:

Something along the lines of:

Image axis angle:

  • hillas.psihillas.image_axis_angle (or position_angle - see below)

Polar coordinates of the centroid:

  • hillas.phihillas.centroid_polar_angle
  • hillas.rhillas.centroid_polar_radius

"Cartesian" (locally) position of the centroid

  • hillas.fov_lathillas.centroid_fov_lat
  • hillas.fov_lonhillas.centroid_fov_lon

We might also consider the term Position Angle, which is how astronomers refer to 2D rotation angles on the sky sphere (not 3D angular coordinates).

Any other suggestions?

@maxnoe
Copy link
Member

maxnoe commented Mar 1, 2023

This is a duplicate of #1078

@maxnoe maxnoe closed this as completed Mar 1, 2023
@kosack
Copy link
Contributor Author

kosack commented Mar 1, 2023

It's not really a duplicate: I saw that issue, but it relates to the algorithm implementation not the naming of the data model . That one should probably be closed as the algorithm was already implemented. The only thing else in that issue is to add more documentation, but it's related. This issue is more about the fact that we have had a few bugs due to how easy it is to misspell or confuse phi/psi

@kosack kosack reopened this Mar 1, 2023
@maxnoe
Copy link
Member

maxnoe commented Mar 1, 2023

It's not really a duplicate: I saw that issue, but it relates to the algorithm implementation not the naming of the data model .

Strange, the first sentence in that issue is:

In a recent PR it was suggested that the Hillas parameters are renamed to be more self-descriptive (#771). I think this was a good suggestion, so I am creating this issue so that it is not forgotten.

@kosack
Copy link
Contributor Author

kosack commented Mar 1, 2023

Yeah, the title of that issue is confusing, It references however this comment, which is basically this issue. We can maybe close #771 and #1078 (though 1078 also has some interesting info in it, even if not a naming scheme).

This issue was meant to replace both,

@maxnoe
Copy link
Member

maxnoe commented Oct 21, 2023

Still think that #1078 is exactly the same as this and it has the relevant discussions, so I am closing here

@maxnoe maxnoe closed this as completed Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants