Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Effective Area 3D #281
base: main
Are you sure you want to change the base?
Effective Area 3D #281
Changes from all commits
14b050e
3d80615
bca9226
6b6ec8d
52f4e44
8056f6e
7e0a1d3
4d5df74
34ecab9
2a6d047
8e4da8b
5e170e4
98da953
f27eef8
777c0b1
44018bc
c80ef58
643958a
d74bd18
554ef57
8ff9e70
be14460
af8a9d1
af76243
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this could lead to confusion, but a 2D azimuthal coordinate and position angle are not quite the same: the polar azimuth starts on the positive X-axis and goes counter-clockwise, while position angle starts on the positive Y-axis and goes clockwise, so there is a 90 deg phase shift and different direction. Perhaps just to avoid this, say "Field-of-view azimuthal (from Y-axis, toward positive x) bin edges...". Probably this should be shown as a figure somewhere in the general PyIRF documentation...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going into the same direction as the comment above about being clear how we define these coordinate systems and adding tests for the cases spelled out by GADF.
One confusing thing here is that the position angle is defined relative to the original coordinate system axes (AltAz or RaDec) but the FoV lon has the opposite direction to the original lon. Which I think makes the polar angle go counter-clockwise when looking at in a common plot of field of view coordinates where the cartesian like axes are FoV lon/lat...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand it correctly the GADF FOV position angle definition is exactly the same as the usual definition of the azimuthal angle in polar coordinates, right? (Angle increasing counter-clockwise)
So following this definition there shouldn't technically be a difference between the two, although I understand that just calling it the azimuthal angle here might still cause confusion.
Since we are following the GADF conventions anyways, would it be agreeable to just change "field of view azimuthal bin edges" to "field of view position angle bin edges" and maybe showing a figure somewhere to clarify as suggested?