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

Correct the sun azimuth angle range within satpy. #2166

Merged
merged 4 commits into from Nov 9, 2022

Conversation

simonrp84
Copy link
Member

At present, the azimuth angles returned for sun and satellite are on different ranges.
satpy.modifiers.angles._get_sun_azimuth_ndarray returns values in the range -180. -> 180. degrees.
satpy.modifiers.angles._get_sensor_angles_ndarray returns values in the range 0. -> 360. degrees.

For things like Rayleigh correction this is not a problem as the angles are normalised on lines 56 and 57 of atmosphere.py. However, it would be better to make this normalisation directly on the solar angles before they're passed to any other functions.
My reasoning for this is:

  1. Users that call either of the azimuth angle calculation functions do not currently know that the angles are on different ranges - this is not documented. This is obviously unwanted and potentially problematic if they don't check the angles themselves before use.
  2. Although only the azimuth difference is used in satpy at the moment this may not always be the case (for more complex atmospheric correction, f.ex). We should therefore be prepared for future changes by ensuring azimuths are correct.

This PR applies a very simple fix (taken from atmosphere.py) to correct the solar azimuth range. Both azimuths will now be in the 0 -> 360 range as expected.

@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #2166 (6d777d1) into main (0c2b965) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2166   +/-   ##
=======================================
  Coverage   94.19%   94.20%           
=======================================
  Files         295      296    +1     
  Lines       45393    45410   +17     
=======================================
+ Hits        42760    42777   +17     
  Misses       2633     2633           
Flag Coverage Δ
behaviourtests 4.65% <0.00%> (-0.01%) ⬇️
unittests 94.85% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
satpy/modifiers/angles.py 99.15% <100.00%> (+<0.01%) ⬆️
satpy/tests/modifier_tests/test_angles.py 100.00% <100.00%> (ø)
satpy/tests/test_modifiers.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@coveralls
Copy link

coveralls commented Aug 5, 2022

Coverage Status

Coverage increased (+0.002%) to 94.798% when pulling 6d777d1 on simonrp84:fix_solar_angle into 0c2b965 on pytroll:main.

@djhoese
Copy link
Member

djhoese commented Aug 5, 2022

@adybbroe or @mraspaud any memory of why the azimuth angle is in a different range in pyorbital?

@simonrp84 I'm tempted to request that the rayleigh correction be updated to only do the % 360 when data is loaded from provided "optional prerequisites" but avoids the extra calculation if the get_angles functions were used. Thoughts?

Also, tests please.

@simonrp84
Copy link
Member Author

I've added some tests to this now. What I did was rearrange the tests as was done in #2175, I moved a bunch of tests from test_modifiers.py into modifier_tests\test_angles.py. I then added a test_solazi_correction function to the test_angles.py file. Hopefully that enables either #2175 or this PR to be merged without causing issues when merging the other PR later.

@djhoese djhoese added the bug label Nov 9, 2022
@djhoese djhoese merged commit 16d95f4 into pytroll:main Nov 9, 2022
@simonrp84 simonrp84 deleted the fix_solar_angle branch November 9, 2022 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants