Adds a common function for geostationary projection / area definition calculations #952

simonrp84 · 2019-10-24T09:38:37Z

This PR adds a new common module that computes area extents / defitions for the geostationary satellites and is intended to replace the existing functions in each of the GEO readers, helping to reduce the amount of redundant code.
The calculations in hrit_base are used as a starting point, and this is inspired by issue #949

Code to-do:

Checks to-do:

Tasks:

Closes Make a common function for geostationnary area_extent computation #949
Tests added and test suite added to parent suite
Tests passed
Passes flake8 satpy

coveralls · 2019-10-24T09:59:33Z

Coverage increased (+0.1%) to 86.852% when pulling b27a12c on simonrp84:geos_proj into e7c56ca on pytroll:master.

codecov · 2019-10-24T09:59:37Z

Codecov Report

Merging #952 into master will increase coverage by 0.1%.
The diff coverage is 93.67%.

@@            Coverage Diff            @@
##           master     #952     +/-   ##
=========================================
+ Coverage   86.75%   86.85%   +0.1%     
=========================================
  Files         179      181      +2     
  Lines       27279    27456    +177     
=========================================
+ Hits        23665    23847    +182     
+ Misses       3614     3609      -5

Impacted Files	Coverage Δ
satpy/tests/reader_tests/test_seviri_l1b_hrit.py	`98.91% <ø> (-0.04%)`	⬇️
satpy/readers/ahi_hsd.py	`96.88% <100%> (-0.05%)`	⬇️
satpy/readers/seviri_l1b_native.py	`67.24% <100%> (+6.17%)`	⬆️
satpy/readers/_geos_area.py	`100% <100%> (ø)`
satpy/tests/reader_tests/__init__.py	`98.33% <100%> (+0.02%)`	⬆️
satpy/readers/electrol_hrit.py	`91.6% <100%> (+0.26%)`	⬆️
satpy/readers/agri_l1.py	`99.16% <100%> (+0.11%)`	⬆️
satpy/readers/ami_l1b.py	`97.47% <100%> (+0.15%)`	⬆️
satpy/tests/reader_tests/test_ahi_hsd.py	`97.82% <100%> (ø)`	⬆️
satpy/tests/reader_tests/test_seviri_l1b_native.py	`93.33% <100%> (+0.62%)`	⬆️
... and 12 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 e7c56ca...b27a12c. Read the comment docs.

djhoese

I'm a little concerned about the get_area_def utility method here. Mainly that to make it generic (to include ABI and AMI) would require the user passing in a and b and h and lon_0 and area name and ...

You end up passing a ton of arguments which makes the code reduction from using this function negligible. There are also special cases like ABI needing the sweep parameter or having special ways of naming things. Maybe the extent calculations should be the only thing that becomes a utility method?

Also note that I put a lot of work in the ABI extent calculations to make the extents nearly the same between all resolutions. So I may not want it to use a generic method.

simonrp84 · 2019-10-24T13:24:25Z

I'm a little concerned about the get_area_def utility method here...you end up passing a ton of arguments which makes the code reduction from using this function negligible.

Agreed, but hopefully the fact that all the computation is will now be done in a single module rather than in each reader will reduce the possibility for calculation errors, as we have all the code in one place.

Also note that I put a lot of work in the ABI extent calculations to make the extents nearly the same between all resolutions. So I may not want it to use a generic method.

Yes, I don't think ABI will benefit from this.

…em for the SEVIRI HRIT reader.

… SEVIRI native reader.

…ull test suite.

mraspaud · 2019-11-05T07:52:32Z

Looking good so far! any news on this ?

simonrp84 · 2019-11-05T08:47:41Z

@mraspaud No progress, funding proposal takes first priority! Hope to finish that sometime today and will then be able to work on research / satpy again.

mraspaud · 2019-11-05T09:07:38Z

No rush! We were just wondering when we would be able to use this feature for the new reader @adybbroe is working on (#886)

…module

mraspaud

LGTM, thanks for doing the tedious work of factorizing and testing for all these geo fileformats!
I have one question regarding AMI (see inline comment), and was wondering if the SAF readers should be using this too, but that could maybe come in a later PR. Looks good otherwise.

satpy/readers/ami_l1b.py

djhoese

Really nice. I had a few things that I'd like changed that I commented on. Otherwise, what are your thoughts on renaming the module to _geos_area.py to make it obvious that this is not a reader and not something normal users should need to look at.

I'm also not really a fan of our (pytroll as a whole) using the function name get_area_definition in multiple places. Is there a better name that we could use?

satpy/readers/geos_area.py

satpy/readers/seviri_l1b_hrit.py

djhoese · 2019-11-08T15:12:53Z

Oh one more thing, thoughts on keyword arguments versus a dictionary of arguments? Makes it so you can document every single keyword argument. You can still do get_area_extents(**pdict).

sfinkens

Nice initiative, thanks! This can probably also be used in goes-imager_nc and jma_hrit. Will create an issue for that. Nevertheless I have a few comments and questions.

satpy/readers/seviri_l1b_hrit.py

satpy/readers/geos_area.py

satpy/tests/reader_tests/test_ahi_hsd.py

satpy/tests/reader_tests/test_geos_area.py

satpy/readers/ahi_hsd.py

…t contains private functions not designed for the user to use independently of satpy readers

simonrp84 · 2019-11-11T09:43:10Z

Really nice. I had a few things that I'd like changed that I commented on. Otherwise, what are your thoughts on renaming the module to _geos_area.py to make it obvious that this is not a reader and not something normal users should need to look at.

Good idea, I've renamed this.

I'm also not really a fan of our (pytroll as a whole) using the function name get_area_definition in multiple places. Is there a better name that we could use?

True, but that's probably something for a separate PR / issue as it also affects the other (non-GEO) sensors.

… commit

simonrp84 · 2019-11-11T09:46:55Z

Oh one more thing, thoughts on keyword arguments versus a dictionary of arguments? Makes it so you can document every single keyword argument. You can still do get_area_extents(**pdict).

To be honest I don't really have the time to make that change just now (as it'll require quite a bit of rewriting) but if you think it's important then I can keep it in mind for when I have some more spare time.

…ite.

sfinkens

Looks good to me, thanks again! Only one tiny nitpick ;)

sfinkens · 2019-11-11T14:14:21Z

satpy/readers/seviri_l1b_hrit.py

@@ -221,7 +224,7 @@ class HRITMSGPrologueEpilogueBase(HRITFileHandler):
    """Base reader for prologue and epilogue files."""

    def __init__(self, filename, filename_info, filetype_info, hdr_info):
-        """Initialize the file handler for prologue and epilogue files."""
+        """Initialize the *logue readers."""


This should be reverted, too

mraspaud · 2019-11-14T11:45:50Z

@djhoese are you happy with the changes now ? or is the documentation changed still not fully reversed ?

djhoese · 2019-11-14T13:53:08Z

@mraspaud At least for me, GitHub still shows the wrong docstring formatting and @sfinkens is right about the other *logue changes that need to be reverted. We should build sphinx HTML before merging this to make sure the docstrings don't cause any issues.

sjoro

Thank you Simon for this very laborious work! This code clean-up / refactoring is very welcome! I can only comment the code bits dealing with SEVIRI and they look good to me and the unittests pass.

djhoese

Nice work @simonrp84. Looks like the docstring stuff is fixed. Thanks for taking the time to clear that up.

simonrp84 added 2 commits October 24, 2019 10:34

Initial GEOS Proj commit

4ed2e10

Update copyright notice

52c8fe1

mraspaud assigned simonrp84 Oct 24, 2019

mraspaud added component:readers enhancement code enhancements, features, improvements labels Oct 24, 2019

djhoese reviewed Oct 24, 2019

View reviewed changes

simonrp84 added 16 commits October 24, 2019 14:27

Complete first version of generic GEO area functions and implement th…

b4d2f15

…em for the SEVIRI HRIT reader.

Add correct version of geos_area

d3404b3

Updated tests for the geos_area, removed unneeded tests from seviri hrit

c2912d8

Bugfixes in seviri HRIT reader, HRV area defs now correct

6adcef9

Update geos_area test to build on appveyor

d34f6e1

Updates geos area test to be compatible with older version of proj

f64181f

bugfix: geos_area int values not explictly converted to float

24dbfd2

Update seviri l1b native reader to use geos_area

591e3c3

Update SEVIRI nc reader to use geos_area, remove unneeded import from…

34d70bd

… SEVIRI native reader.

Update Electro-L/GOMS reader to support the geos_area module.

da5fed3

Change scandir parameter in geos_area to be a string rather than an int

1f696c2

Updates to tests for geos_area scan direction

cd54f0e

Ensure float conversion for geos_area

1075b58

Update AGRI tests so that they can also be run independently of the f…

c8cac93

…ull test suite.

Update AGRI L1 reader to support the new geos_area module

6254d82

Remove trailing whitespace from AGRI

39921c1

simonrp84 added 2 commits November 5, 2019 17:09

Update AHI reader and test to support the new common GEOS projection …

38c8e02

…module

Update to latest satpy master

99a513d

simonrp84 added 3 commits November 7, 2019 09:11

Whitespace fix in __init__.py

ef5cc92

Update seviri l1b native tests to work with new geos_proj method

b38d51e

geos_proj updates to support the GK2A/AMI satellite/instrument.

86af65b

simonrp84 marked this pull request as ready for review November 8, 2019 11:27

simonrp84 requested review from mraspaud, sfinkens and sjoro as code owners November 8, 2019 11:27

mraspaud reviewed Nov 8, 2019

View reviewed changes

satpy/readers/ami_l1b.py Show resolved Hide resolved

djhoese requested changes Nov 8, 2019

View reviewed changes

satpy/readers/geos_area.py Outdated Show resolved Hide resolved

satpy/readers/geos_area.py Outdated Show resolved Hide resolved

satpy/readers/seviri_l1b_hrit.py Outdated Show resolved Hide resolved

sfinkens requested changes Nov 8, 2019

View reviewed changes

sfinkens mentioned this pull request Nov 8, 2019

Factorize area def computation in jma_hrit #960

Closed

simonrp84 added 5 commits November 11, 2019 08:57

Revert accidental documentation style changes in geos_proj

addc504

Documentation and style changes to geos_area.py

987cb04

Update geos_area and ahi_hsd tests to use numpy allclose

012de47

Update geos_proj tests to check also a subset of the full disk.

c6b5c4c

Rename the geos_area.py with a leading underscore, to indicate that i…

6a47189

…t contains private functions not designed for the user to use independently of satpy readers

Fix some style errors in test_ahi_hsd that snuck through the previous…

d679397

… commit

Correct AHI HSD scan pattern to be North->South rather than the oppos…

fbbd761

…ite.

sfinkens approved these changes Nov 11, 2019

View reviewed changes

simonrp84 added 2 commits November 14, 2019 15:08

Revert documentation change in the new geos_proj

f1eb95f

Revert documentation change in the new geos_proj

b27a12c

sjoro approved these changes Nov 21, 2019

View reviewed changes

mraspaud requested a review from djhoese November 21, 2019 08:20

djhoese approved these changes Nov 21, 2019

View reviewed changes

mraspaud merged commit 1fbfda5 into pytroll:master Nov 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a common function for geostationary projection / area definition calculations #952

Adds a common function for geostationary projection / area definition calculations #952

simonrp84 commented Oct 24, 2019 •

edited by mraspaud

Loading

coveralls commented Oct 24, 2019 •

edited

Loading

codecov bot commented Oct 24, 2019 •

edited

Loading

djhoese left a comment

simonrp84 commented Oct 24, 2019 •

edited

Loading

mraspaud commented Nov 5, 2019

simonrp84 commented Nov 5, 2019

mraspaud commented Nov 5, 2019

mraspaud left a comment

djhoese left a comment

djhoese commented Nov 8, 2019

sfinkens left a comment

simonrp84 commented Nov 11, 2019 •

edited

Loading

simonrp84 commented Nov 11, 2019

sfinkens left a comment

sfinkens Nov 11, 2019

mraspaud commented Nov 14, 2019

djhoese commented Nov 14, 2019

sjoro left a comment

djhoese left a comment

Adds a common function for geostationary projection / area definition calculations #952

Adds a common function for geostationary projection / area definition calculations #952

Conversation

simonrp84 commented Oct 24, 2019 • edited by mraspaud Loading

coveralls commented Oct 24, 2019 • edited Loading

codecov bot commented Oct 24, 2019 • edited Loading

Codecov Report

djhoese left a comment

Choose a reason for hiding this comment

simonrp84 commented Oct 24, 2019 • edited Loading

mraspaud commented Nov 5, 2019

simonrp84 commented Nov 5, 2019

mraspaud commented Nov 5, 2019

mraspaud left a comment

Choose a reason for hiding this comment

djhoese left a comment

Choose a reason for hiding this comment

djhoese commented Nov 8, 2019

sfinkens left a comment

Choose a reason for hiding this comment

simonrp84 commented Nov 11, 2019 • edited Loading

simonrp84 commented Nov 11, 2019

sfinkens left a comment

Choose a reason for hiding this comment

sfinkens Nov 11, 2019

Choose a reason for hiding this comment

mraspaud commented Nov 14, 2019

djhoese commented Nov 14, 2019

sjoro left a comment

Choose a reason for hiding this comment

djhoese left a comment

Choose a reason for hiding this comment

simonrp84 commented Oct 24, 2019 •

edited by mraspaud

Loading

coveralls commented Oct 24, 2019 •

edited

Loading

codecov bot commented Oct 24, 2019 •

edited

Loading

simonrp84 commented Oct 24, 2019 •

edited

Loading

simonrp84 commented Nov 11, 2019 •

edited

Loading