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 satellite altitude being in kilometers in ABI L2 reader #1631

Merged
merged 3 commits into from
Apr 16, 2021

Conversation

aronnem
Copy link
Contributor

@aronnem aronnem commented Apr 9, 2021

The attribute data assumes the satellite altitude is stored
in meters, while the ABI L2 data stores this value in km;
we simply need to insert factor of 1000 to convert the units.
This matches what happens in the ABI L1b reader.

The attribute data assumes the satellite altitude is stored
in meters, while the ABI L2 data stores thisvalue in km;
we simply need to insert factor of 1000 to convert the units.
This matches what happens in the ABI L1b reader.
@aronnem aronnem requested review from djhoese and mraspaud as code owners April 9, 2021 20:47
@ghost
Copy link

ghost commented Apr 9, 2021

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

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

@mraspaud
Copy link
Member

@yufeizhu600

@djhoese
Copy link
Member

djhoese commented Apr 10, 2021

Thanks for the PR. Just to be clear @aronnem, this is a difference between the L1b and L2 files or is this a change that is done in the L1b reader but wasn't being done in the L2 reader?

@yufeizhu600
Copy link
Contributor

I have checked the GOES-R PUG, the satellite height is in km.

nominal_satellite_height 
value = 35786.023

@djhoese I have checked the code, @sfinkens has corrected the unit error in the abi l1b reader two years ago.
https://github.com/pytroll/satpy/commit/b9f92ad58ea4e09485e97aa90b392a0bc606c20a

@aronnem. Thanks for the PR, could you please also update the unit tests.

@aronnem
Copy link
Contributor Author

aronnem commented Apr 13, 2021

@yufeizhu600 I can update the tests, but need a little more direction (sorry, I am new at this.) I see a couple spots in reader_tests, for the data mockup and the test, for example:

https://github.com/pytroll/satpy/blob/master/satpy/tests/reader_tests/test_abi_l2_nc.py#L70

Is that all I need to update?

@djhoese
Copy link
Member

djhoese commented Apr 13, 2021

Yes @aronnem that will update the fake data that goes into the tests so the test will also need to be updated.

  1. Update the fake data:

    "nominal_satellite_height": np.array(35786020.),

  2. Update the test expected values:

    'satellite_altitude': 35786020.,
    'satellite_latitude': 0.0,
    'satellite_longitude': -89.5,

  3. Run the tests by doing pytest satpy/tests/reader_tests/test_abi_l2_nc.py and make sure everything passes.

@codecov
Copy link

codecov bot commented Apr 13, 2021

Codecov Report

Merging #1631 (4571783) into master (fac76a7) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1631      +/-   ##
==========================================
+ Coverage   92.61%   92.62%   +0.01%     
==========================================
  Files         258      258              
  Lines       37761    37810      +49     
==========================================
+ Hits        34972    35022      +50     
+ Misses       2789     2788       -1     
Flag Coverage Δ
behaviourtests 4.81% <ø> (-0.01%) ⬇️
unittests 92.76% <ø> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
satpy/readers/abi_l2_nc.py 70.00% <ø> (ø)
satpy/tests/reader_tests/test_abi_l2_nc.py 100.00% <ø> (ø)
satpy/tests/test_data_download.py 98.66% <0.00%> (-1.34%) ⬇️
satpy/aux_download.py 97.60% <0.00%> (-0.70%) ⬇️
satpy/tests/reader_tests/test_aapp_l1b.py 100.00% <0.00%> (ø)
satpy/readers/aapp_l1b.py 89.82% <0.00%> (+0.44%) ⬆️
satpy/modifiers/_crefl.py 100.00% <0.00%> (+1.11%) ⬆️
satpy/modifiers/geometry.py 87.09% <0.00%> (+3.22%) ⬆️

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 fac76a7...4571783. Read the comment docs.

@aronnem
Copy link
Contributor Author

aronnem commented Apr 13, 2021

@yufeizhu600 @djhoese yes, I only needed to make one change. The mockup dataset needs to match the real netCDF (which has the altitude in km).

@yufeizhu600
Copy link
Contributor

@aronnem, thanks for updating, it looks goods to me. Congrats for the first PR, don't forget to add your name to the authors document.
https://github.com/pytroll/satpy/blob/master/AUTHORS.md
You can update it in your PR and the authors doc will be updated as your PR merged.

@djhoese
Copy link
Member

djhoese commented Apr 13, 2021

If the tests weren't failing before and they aren't failing now, then the change made to the reader isn't being tested. The test needs to be updated.

@mraspaud
Copy link
Member

mraspaud commented Apr 13, 2021

The tests were failing before 115d4f2

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Ah, right you are. Looks good then. Thanks.

@djhoese
Copy link
Member

djhoese commented Apr 13, 2021

Oh but as @yufeizhu600 said, could you add yourself to the AUTHORS.md?

@djhoese djhoese changed the title bugfix for satellite altitude in ABI L2 reader Fix satellite altitude being in kilometers in ABI L2 reader Apr 13, 2021
@mraspaud mraspaud merged commit 7aa706b into pytroll:master Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants