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

Fix get_observer_look for satellite exactly at nadir #77

Merged
merged 9 commits into from Sep 23, 2021

Conversation

ninahakansson
Copy link

@ninahakansson ninahakansson commented Apr 12, 2021

When the satellite is exactly above the observer the satellite
elevation angle should be 90 degrees and the azimuth angle is undefined.
Set azimuth angle to 180 (initially 0) when satellite elevation is 90 degrees.
Also handle the case when top_z / rg_ is larger than 1.0 due to rounding.
Previously nans were often returned for satellite elevation 90 degrees.
The Orbital class also has a get_observer_look function which does not handle
dask and xarray. This function has not been updated.

When the satellite is exactly above the observer the satellite
elevation angle is 90 degrees the azimuth angle is undefined.

Set asimuth angle to 0 when satellite elevation is 90 degrees.

Also handle the case when top_z / rg_ is larger than 1.0 due to rounding.
@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #77 (ca1d4d2) into main (6b7e685) will increase coverage by 0.35%.
The diff coverage is 98.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   85.31%   85.67%   +0.35%     
==========================================
  Files          13       13              
  Lines        1893     1947      +54     
==========================================
+ Hits         1615     1668      +53     
- Misses        278      279       +1     
Flag Coverage Δ
unittests 85.67% <98.68%> (+0.35%) ⬆️

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

Impacted Files Coverage Δ
pyorbital/tests/test_orbital.py 96.21% <98.57%> (+0.76%) ⬆️
pyorbital/orbital.py 86.97% <100.00%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b7e685...ca1d4d2. Read the comment docs.

@ghost
Copy link

ghost commented Apr 12, 2021

Congratulations 🎉. DeepCode analyzed your code in 3.465 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@adybbroe adybbroe added this to In progress in PCW Spring 2021 May 20, 2021
This happens when satellite is exactly at observer nadir.
The problem depends on rounding and varies between array types:
numpy, dask, and xarray.

Removed the check for sat_at_nadir as is not needed any longer.
@ninahakansson ninahakansson marked this pull request as ready for review May 20, 2021 16:10
@simonrp84
Copy link
Member

While slightly out-of-scope for this PR, do you know if this problem that you've fixed here would also affect solar angles?

@mraspaud mraspaud moved this from In progress to Review in progress in PCW Spring 2021 May 21, 2021
@ninahakansson
Copy link
Author

Good question!

It should not happen for solar azimuth angles as pyorbital in astronomy get_alt_az uses np.artan2 that handles special values
separately:
https://numpy.org/doc/stable/reference/generated/numpy.arctan2.html
So maybe I should just update to use that function also for the satellite angles?

For the sun zenith angle the formulas are different so I don't think the arcsine for values larger than 1 could happen.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for unrooting this bug!

I think some parts can be simplified, see inline comments.

Good work!

pyorbital/orbital.py Outdated Show resolved Hide resolved
Comment on lines 150 to 151
top_z_divided_by_rg_ = top_z / rg_
el_ = np.arcsin(top_z_divided_by_rg_)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using clip might help avoid the extra filtering later:

Suggested change
top_z_divided_by_rg_ = top_z / rg_
el_ = np.arcsin(top_z_divided_by_rg_)
top_z_divided_by_rg_ = top_z / rg_
top_z_divided_by_rg_ = top_z_divided_by_rg_.clip(max=1)
el_ = np.arcsin(top_z_divided_by_rg_)

pyorbital/orbital.py Outdated Show resolved Hide resolved
# And azimuth undefined when elevation is 90 degrees
if has_dask and isinstance(az_data, da.Array):
el_data = da.where(top_z_divided_by_rg_ > 1.0, np.pi/2, el_data)
az_data = da.where(el_data == np.pi / 2, 0, az_data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this filtering and the undet_azi higher up, would np.nan_to_num be helpfull here? or do we still want some azimuths to be nans?

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Unexpected Nans in get_observer_look_no_tle
4 participants