Changed azimuth angle range to ]-180, 180] #35
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As angles are saved as np.int16, with gain=0.01 and offset=0, all angles will now fit within the datatype.
|
LGTM |
sfinkens
approved these changes
Oct 16, 2019
mraspaud
reviewed
Oct 16, 2019
There was a problem hiding this comment.
Thank you for fixing the angles interval. I have a couple of comments that are more from the nitpicking domain than anything else, but I'll write them here for reference:
- The tests added are only indirectly testing the new code. In the spirit of unit testing, shouldn't a function be created (eg
centered_modulus) that returns a centered modulus and test that first ? The tests provided here are absolutely relevant though, that's not a problem :) - It should be documented somewhere that the returned angles are going to be in the range ]-180, 180]
Otherwise, it looks good !
The centered_modulus function was suggested by Martin.
mraspaud
requested changes
Oct 17, 2019
pygac/gac_reader.py
Outdated
Comment on lines
456
to
471
| """Get azimuth and zenith angles. | ||
|
|
||
| Azimuth angle definition is the same as in pyorbital, but with | ||
| different units (degrees not radians for sun azimuth angles) | ||
| and different ranges. | ||
|
|
||
| Angle definitions: | ||
| azimuth angles: degree clockwise from north in range ]-180, 180] | ||
| zenith angles: degree between zenith and sensor or sun in range [0, 90] | ||
| absolute azimuth angle difference between sun and sensor: in [0, 180] | ||
|
|
||
| Returns: | ||
| azimuth angles for sun and satellite (sun_azi, sat_azi) | ||
| zenith angles for sun and satellite (sun_zenith, sat_zenith) | ||
| absolute azimuth angle difference (rel_azi) | ||
|
|
There was a problem hiding this comment.
Could you format the docstring in google-style? see here for an example: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html
mraspaud
approved these changes
Oct 17, 2019
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Updated azimuth angles range to be in ]-180, 180].
Before one angle was in ]-180, 180] and the other in [0, 360]
The range ]-180, 180] was selected as all angles then can be saved
using the current data type, offset and gain.