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

Update AGRI/L1 geolocation #1782

Merged
merged 12 commits into from Sep 1, 2021
Merged

Update AGRI/L1 geolocation #1782

merged 12 commits into from Sep 1, 2021

Conversation

simonrp84
Copy link
Member

@simonrp84 simonrp84 commented Aug 2, 2021

As described in #1773, the geolocation is incorrect for FY4A/AGRI. This is due to the code containing a +2000 within the area definition extent, which was originally required to ensure the geolocation was correct.
However, since that code was added the geos_area code for satpy has been updated and the factor of 2000 is no longer required. This PR removes the unneeded code and means that AGRI data will be better geolocated.

FengYun-4A_C03_reflectance_imshow_201903170500_cartopy_fix
FengYun-4A_C03_reflectance_imshow_201903170500_cartopy_orig

(edit) I also removed an elif that codefactor was moaning about. This doesn't affect how the code runs.

@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #1782 (da26750) into main (a76c17e) will increase coverage by 0.02%.
The diff coverage is 99.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1782      +/-   ##
==========================================
+ Coverage   92.90%   92.92%   +0.02%     
==========================================
  Files         265      265              
  Lines       38942    39092     +150     
==========================================
+ Hits        36178    36327     +149     
- Misses       2764     2765       +1     
Flag Coverage Δ
behaviourtests 4.76% <0.00%> (-0.02%) ⬇️
unittests 93.46% <99.54%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
satpy/readers/agri_l1.py 99.15% <99.15%> (ø)
satpy/tests/reader_tests/test_agri_l1.py 100.00% <100.00%> (ø)
satpy/tests/reader_tests/test_abi_l1b.py 99.25% <0.00%> (-0.75%) ⬇️
satpy/writers/__init__.py 88.38% <0.00%> (-0.46%) ⬇️
satpy/readers/clavrx.py 90.26% <0.00%> (-0.34%) ⬇️
satpy/tests/test_yaml_reader.py 99.54% <0.00%> (-0.01%) ⬇️
satpy/scene.py 92.32% <0.00%> (ø)
satpy/_config.py 98.59% <0.00%> (ø)
satpy/multiscene.py 91.10% <0.00%> (ø)
satpy/readers/abi_base.py 94.59% <0.00%> (ø)
... and 27 more

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 a76c17e...da26750. Read the comment docs.

@coveralls
Copy link

coveralls commented Aug 2, 2021

Coverage Status

Coverage increased (+0.02%) to 93.408% when pulling da26750 on simonrp84:fy4a_shifted into a76c17e on pytroll:main.

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.

Github seems very confused in its diff. Hard to tell what was changed, but given that you have the whole history of the code laid out in your description I assume this looks right. Looks like @zxdawn offered to test it soon. After he verifies that it works we can merge it. Thanks @simonrp84!

@djhoese
Copy link
Member

djhoese commented Aug 2, 2021

Jobs failed due to hungup HTTP connection. I've restarted them.

@simonrp84
Copy link
Member Author

simonrp84 commented Aug 2, 2021

No idea what's up with the diff.
I made two changes:

  1. elif -> if on line 73 of original file.
  2. area_extent = (area_extent[0] + 2000, area_extent[1], area_extent[2] + 2000, area_extent[3]) -> area_extent = (area_extent[0], area_extent[1], area_extent[2], area_extent[3]) on line 163.

@djhoese
Copy link
Member

djhoese commented Aug 2, 2021

@simonrp84 the website building is failing because scipy change the structure of their docs website. Can you edit this file in your PR and drop the /reference in the URL:

'scipy': ('https://docs.scipy.org/doc/scipy/reference', None),

Edit: Commit message something like "Update intersphinx URL for scipy"

@simonrp84
Copy link
Member Author

Done as you requested. Codefactor complaining now, shall I remove that pass @djhoese? I don't know this part of the code well enough to be confident, but it looks reasonable to remove.

@djhoese
Copy link
Member

djhoese commented Aug 2, 2021

Don't worry about it. This is the sphinx documentation configuration file. While this method does have a docstring so pass could get removed, I don't think you need to put any more work into this.

@simonrp84
Copy link
Member Author

Ok, in that case I think this is ready to merge.

@mraspaud
Copy link
Member

mraspaud commented Aug 3, 2021

Could we add a test to make sure such a problem doesn't happen again?

@zxdawn
Copy link
Member

zxdawn commented Aug 3, 2021

@simonrp84 Tested it for the newest 500m C02 data and it's good:

image
image

Copy link
Member

@sfinkens sfinkens left a comment

Choose a reason for hiding this comment

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

Thanks @simonrp84 for fixing this! A test would be great so that I know if I break something when looking at the area extents again.

satpy/readers/agri_l1.py Outdated Show resolved Hide resolved
@mraspaud
Copy link
Member

I'm finished with the area test and the refactor. @zxdawn @simonrp84 If you have time to have a quick look...

Copy link
Member

@sfinkens sfinkens left a comment

Choose a reason for hiding this comment

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

@mraspaud Excellent refactoring work!

@mraspaud mraspaud merged commit 3be3d72 into pytroll:main Sep 1, 2021
@simonrp84 simonrp84 deleted the fy4a_shifted branch September 2, 2021 08:22
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.

[Question] Geolocation information of FengYun4A (FY-4A) AGRI L1B data
6 participants