Harmonise AreaDefinition namings in EUM geos readers and sort geos areas in areas.yaml #1485

ameraner · 2020-12-14T12:40:50Z

This PR contributes to #1248 by standardizing the way AreaDefinition's area_id, description and proj_id are returned in geostationary data readers of EUMETSAT data (L1 SEVIRI and FCI).
It follows the convention <platform>_<instrument>_<service>_<resolution> for the area_id. A generic formatting function get_geos_area_naming is implemented in _geos_area.

New standard areas for SEVIRI and FCI are added to areas.yaml.

It also starts the work on #819 by sorting the geostationary areas.
Changes "globe" to "disk" in area descriptions to close #1187.

Tests added
Passes flake8 satpy
Fully documented

…sary proj_id and description from test dicts

codecov · 2020-12-14T16:53:13Z

Codecov Report

Merging #1485 (c2f56cd) into master (45843de) will increase coverage by 0.00%.
The diff coverage is 94.59%.

@@           Coverage Diff           @@
##           master    #1485   +/-   ##
=======================================
  Coverage   90.87%   90.88%           
=======================================
  Files         241      241           
  Lines       35113    35173   +60     
=======================================
+ Hits        31908    31966   +58     
- Misses       3205     3207    +2

Flag	Coverage Δ
behaviourtests	`4.48% <0.00%> (-0.01%)`	⬇️
unittests	`91.35% <94.59%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/readers/seviri_base.py	`95.18% <ø> (-0.09%)`	⬇️
satpy/readers/seviri_l1b_nc.py	`49.57% <20.00%> (-0.43%)`	⬇️
satpy/readers/_geos_area.py	`100.00% <100.00%> (ø)`
satpy/readers/eum_base.py	`73.46% <100.00%> (+2.35%)`	⬆️
satpy/readers/fci_l1c_fdhsi.py	`97.51% <100.00%> (+0.03%)`	⬆️
satpy/readers/seviri_l1b_hrit.py	`90.93% <100.00%> (+0.05%)`	⬆️
satpy/readers/seviri_l1b_native.py	`79.11% <100.00%> (-0.20%)`	⬇️
satpy/tests/reader_tests/test_eum_base.py	`100.00% <100.00%> (ø)`
satpy/tests/reader_tests/test_geos_area.py	`100.00% <100.00%> (ø)`
satpy/tests/reader_tests/test_seviri_base.py	`100.00% <100.00%> (ø)`
... and 2 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 45843de...c2f56cd. Read the comment docs.

djhoese

Wow, so many changes. Nice job. Had a few small style questions and comments.

djhoese · 2020-12-14T17:24:55Z

satpy/etc/areas.yaml

@@ -1,3 +1,212 @@
+# This file contains a set of pre-defined areas to be used for resampling purposes.
+
+# ----------------------------------------------------------------------------------------------------------------------


Thoughts on an 80 character limit for this file? There are ways in YAML to make multiline strings if we need to (for the area "description" if those get long.

I have no opinion on this, so as you wish.

I guess we could go with:

description: >- MSG SEVIRI Full Earth Scanning service area definition with 3 km resolution

Yeah I just noticed that it was hard to read all the section headers because of how long they were. Could have been my browser window width or GitHub imposing a character/column limit.

Done! Noticed that simply splitting the lines without the >- works the same (folding with a space, no end newline).

djhoese · 2020-12-14T17:27:35Z

satpy/etc/areas.yaml

+    lower_left_xy: [-4823148.089050828, 1969764.6783588605]
+    upper_right_xy: [4178061.408400173, 5570248.477339261]
+
+EastEurope:


Is this an area that existed before? If so, did it have the same name? Regardless, EastEurope, EuropeCanary, and SouthAmerica are pretty generic names for geostationary specific areas. Obviously, whatever amount of South America SEVIRI sees in its geostationary full disk isn't going to be the best for other datasets that are more "central" to South America.

Ah I see now that these were old areas. Did we decide we'd come up with better names for these in a later PR?

Yeah, those are all old areas. And yes, I think another PR is needed for the general cleanup/renaming, and possibly adding a deprecation warning. For now, I relegated to deprecation only the old full-disk ones.

gerritholl · 2020-12-14T17:35:42Z

satpy/etc/areas.yaml

+# ----------------------------------------------------------------------------------------------------------------------
+# ------------------------------------------------ Areas to be deprecated ----------------------------------------------
+# ----------------------------------------------------------------------------------------------------------------------
+# This section contains areas that are obsolete.


Is it possible for satpy to issue a warning when someone uses an obsolete area name?

Not currently, but I think we could add it. The cases where someone does new_scn = scn.resample('obsolete_area') would be very easy to handle. The cases where someone is manually specifying a path to the areas.yaml file and requesting it might be harder since there are a few utilities doing that and most are in pyresample which doesn't know or care about the areas defined in satpy.

I guess yes, e.g. a check for deprecated areas could be placed in satpy.resample.get_area_def(), which is what is used to get the destination AreaDefinition out of the yaml when resampling... TBD in #819

satpy/readers/_geos_area.py

sfinkens

Good work! Just one request: Since we discovered during the PCW that the SEVIRI Native area extent is slightly different from msg_seviri_fes_1/3km, I think it would be nice to mention that in NativeMSGFileHandler.get_area_def.

…a_def

ameraner · 2020-12-16T11:55:25Z

@sfinkens Thanks! Good idea, I added a notice in the native and netcdf readers get_area_def

sjoro

LGTM. thanks for sorting this out and also for fixing the documentation weblinks!

mraspaud

Looks like the changes in the seviri nc reader aren't covered, otherwise LGTM

satpy/etc/areas.yaml

mraspaud · 2020-12-16T13:06:59Z

Oh, also, proj_id should really be deprecated, so should we just set it to an empty value here instead?

ameraner · 2020-12-16T13:22:44Z

@mraspaud I was wondering the same about the proj_id. I'm ok with setting it to empty and make things easier!

Regarding the NetCDF tests: yes the changes are not covered, because there's almost no test at all for the NetCDF reader. So I figured I would leave it for now, planning to add comprehensive tests in another dedicated PR. Is it ok?

mraspaud · 2020-12-16T13:23:47Z

I'm ok with leaving the nc part uncovered for now.

sfinkens · 2020-12-16T13:42:25Z

satpy/readers/seviri_l1b_native.py

+        Note that the AreaDefinition area extents returned by this function for Native data will be slightly
+        different compared to the area extents returned by the SEVIRI HRIT reader.
+        This is due to slightly different pixel size values when calculated using the data available in the files. E.g.
+        for the 3 km grid:
+
+        ``Native: data15hd['ImageDescription']['ReferenceGridVIS_IR']['ColumnDirGridStep'] == 3000.4031658172607``
+        ``HRIT:                            np.deg2rad(2.**16 / pdict['lfac']) * pdict['h'] == 3000.4032785810186``
+
+        This results in the Native 3 km full-disk area extents being approx. 20 cm shorter in each direction.
+
+        The method for calculating the area extents used by the HRIT reader (CFAC/LFAC mechanism) keeps the
+        highest level of numeric precision and is used as reference by EUM. For this reason, the standard area
+        definitions defined in the `areas.yaml` file correspond to the HRIT ones.
+


Excellent, thanks!

Conflicts: satpy/readers/seviri_base.py satpy/readers/seviri_l1b_hrit.py satpy/readers/seviri_l1b_native.py satpy/readers/seviri_l1b_nc.py

…eprecation note

ameraner · 2020-12-16T15:36:33Z

@mraspaud added a commit that replaces all proj_id as empty strings and adds a note in the formatting function about the deprecation.

…nition

mraspaud

LGTM

ameraner · 2020-12-16T16:52:31Z

Thanks all for your help and discussions!

ameraner added 11 commits December 11, 2020 16:26

implement naming functions in eum_base

a24f21f

make native reader use new naming functions

40d94eb

make fci reader use new naming functions

b3e1b2c

make hrit reader use new naming functions

713b547

make nc reader use new naming functions and update documents links

5be2072

move methods from eum_base to _geos_area

2eab9b8

make naming methods more general and move information input to readers

8f86327

sort areas.yaml for geos areas

351c28c

fix service naming

916cb14

add seviri areas.yaml and add deprecation comments

6914118

replace globe for disk in area descriptions to close pytroll#1187

24c35d4

ameraner changed the title ~~Harmonise AreaDefinition namings in EUM geos readers and add new and sort geos areas in areas.yaml~~ Harmonise AreaDefinition namings in EUM geos readers and sort geos areas in areas.yaml Dec 14, 2020

ameraner added 9 commits December 14, 2020 14:12

remove unnecessary else

44591de

use get for service mode dictionary retrieval and add "unknown" fallback

7c31f2c

change tests to check area_id rather than proj_id, and remove unneces…

dcb70ff

…sary proj_id and description from test dicts

fix native tests

f82773f

move get_service_mode tests to eum_base and update

4b23ba8

add unknown input tests for get_service_mode

287637c

fix hrit tests

3804dbd

add tests for new function in _geos_area and update docstrings

ce8ef99

add description for South America

19c273c

ameraner added 2 commits December 14, 2020 17:55

update resolution docstring

5170605

add test also for second window in HRV HRIT

d62aaf2

ameraner marked this pull request as ready for review December 14, 2020 17:05

ameraner requested review from adybbroe, djhoese, mraspaud and sfinkens as code owners December 14, 2020 17:05

ameraner requested a review from sjoro December 14, 2020 17:06

fix documentation link for seviri_base

d11ce5d

ameraner mentioned this pull request Dec 14, 2020

Broken links to new EUMETSAT website #1484

Closed

djhoese reviewed Dec 14, 2020

View reviewed changes

gerritholl reviewed Dec 14, 2020

View reviewed changes

strandgren reviewed Dec 15, 2020

View reviewed changes

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

ameraner added 2 commits December 15, 2020 10:14

add service_desc to docstring in get_geos_area_naming

3fbe54c

limit areas.yaml line characters to 79

b23d8b6

sfinkens reviewed Dec 15, 2020

View reviewed changes

add notices about different area extents in native and netcdf get_are…

c260490

…a_def

sjoro approved these changes Dec 16, 2020

View reviewed changes

mraspaud reviewed Dec 16, 2020

View reviewed changes

satpy/etc/areas.yaml Outdated Show resolved Hide resolved

sfinkens approved these changes Dec 16, 2020

View reviewed changes

ameraner added 3 commits December 16, 2020 15:24

Merge branch 'master' into feature-harmonise_EUM_area_naming

4b32101

Conflicts: satpy/readers/seviri_base.py satpy/readers/seviri_l1b_hrit.py satpy/readers/seviri_l1b_native.py satpy/readers/seviri_l1b_nc.py

fix sve area description

af33f67

use empty strings for proj_id, remove formatting of proj_id and add d…

69eb991

…eprecation note

delete comment on proj_id on wrong line and add note to get_area_defi…

c2f56cd

…nition

mraspaud approved these changes Dec 16, 2020

View reviewed changes

mraspaud merged commit 700ab42 into pytroll:master Dec 16, 2020

mraspaud assigned ameraner Dec 16, 2020

mraspaud added the enhancement code enhancements, features, improvements label Dec 16, 2020

ameraner deleted the feature-harmonise_EUM_area_naming branch March 1, 2021 16:54

sfinkens mentioned this pull request Dec 12, 2021

Harmonize SEVIRI area definitions #1192

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harmonise AreaDefinition namings in EUM geos readers and sort geos areas in areas.yaml #1485

Harmonise AreaDefinition namings in EUM geos readers and sort geos areas in areas.yaml #1485

ameraner commented Dec 14, 2020 •

edited

codecov bot commented Dec 14, 2020 •

edited

djhoese left a comment

djhoese Dec 14, 2020

ameraner Dec 14, 2020

djhoese Dec 14, 2020

ameraner Dec 15, 2020

djhoese Dec 14, 2020

djhoese Dec 14, 2020

ameraner Dec 14, 2020

gerritholl Dec 14, 2020

djhoese Dec 14, 2020

ameraner Dec 14, 2020

sfinkens left a comment

ameraner commented Dec 16, 2020

sjoro left a comment

mraspaud left a comment

mraspaud commented Dec 16, 2020

ameraner commented Dec 16, 2020

mraspaud commented Dec 16, 2020

sfinkens Dec 16, 2020

ameraner commented Dec 16, 2020

mraspaud left a comment

ameraner commented Dec 16, 2020

		@@ -1,3 +1,212 @@
		# This file contains a set of pre-defined areas to be used for resampling purposes.

		# ----------------------------------------------------------------------------------------------------------------------

Harmonise AreaDefinition namings in EUM geos readers and sort geos areas in areas.yaml #1485

Harmonise AreaDefinition namings in EUM geos readers and sort geos areas in areas.yaml #1485

Conversation

ameraner commented Dec 14, 2020 • edited

codecov bot commented Dec 14, 2020 • edited

Codecov Report

djhoese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfinkens left a comment

Choose a reason for hiding this comment

ameraner commented Dec 16, 2020

sjoro left a comment

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud commented Dec 16, 2020

ameraner commented Dec 16, 2020

mraspaud commented Dec 16, 2020

Choose a reason for hiding this comment

ameraner commented Dec 16, 2020

mraspaud left a comment

Choose a reason for hiding this comment

ameraner commented Dec 16, 2020

ameraner commented Dec 14, 2020 •

edited

codecov bot commented Dec 14, 2020 •

edited