MTG: get projection and extent information from file #845

gerritholl · 2019-07-04T09:34:51Z

In the MTG FCI FDHSI L1C reader, get the projection coordinates and
extent information from file. For the projection coordinates, get those
from /data/mtg_geos_projection rather than from /state/processor, as
is consistent with the documentation. The extent information was
previously hardcoded but now obtained from data/channel/measured/x and
data/channel/measured/y, which contain this information in radians.
This commit fixes #840 although there are still some issues with the
resulting image that need to be resolved.

gerritholl · 2019-07-04T09:36:14Z

I'm not sure how to write a test for "coastlines look correct in image". It could be done with some fancy image processing but it may be more effort than it's worth?

gerritholl · 2019-07-04T09:38:31Z

The image still has some problems, it looks like this now:

The coastlines are correct, but there are two kinds of artefacts:

concentric projection artefacts
zonal artefacts between the different chunks in the satellite image

mraspaud · 2019-07-04T09:44:28Z

try setting the radius_of_influence parameter to 10000 when you resample

mraspaud · 2019-07-04T09:48:06Z

As for the test, you could create some fake data with special pixel values at some landmarks, and then make sure the landmarks (with special pixel values) are in the right place in the resampled image.

mraspaud · 2019-07-04T09:50:48Z

Regarding the zonal artefacts, are you sure the area extent is correct, ie that is extends to the corner of the corner point ?

gerritholl · 2019-07-04T09:51:49Z

Thanks for the advice. I will look into those and then I need to look into the zonal artefacts, I seem to recall something about defining locations as pixel centres vs. pixel corners.

mraspaud · 2019-07-04T09:52:35Z

yes, I see in your code that you use pixel centers...

gerritholl · 2019-07-04T12:18:53Z

As for the test, you could create some fake data with special pixel values at some landmarks, and then make sure the landmarks (with special pixel values) are in the right place in the resampled image.

Are there examples for that in other reading routine testers?

mraspaud · 2019-07-04T12:21:55Z

No, it's never been done :) but maybe you don't need to got to these length to test the navigation of this reader. I would suggest that you just check the area extent computed from the parameters given in the file against what you have determined is correct experimentally.

gerritholl · 2019-07-04T13:01:33Z

After 0a7d0e2:

it now appears to be correct as far as I can see.

I find actually unit testing this difficult as I'm not sure how to verify that a pixel in my mock data ends up where I expect it to end up, since I don't have an independent easy way of calculating the test reference.

mraspaud · 2019-07-04T13:04:40Z

Isn't the data shifted a bit to the right ? looks like the coastlines don't really match on the tip of Sweden.
As for tests, just record the correct values for area extent you find with the test data and use them as a reference in the unit tests

gerritholl · 2019-07-04T13:07:45Z

Hmm, looking at Bornholm it looks shifted indeed, I thought that the Dutch coast looked fine. That's puzzling.

coveralls · 2019-07-04T14:43:02Z

Coverage increased (+0.009%) to 89.545% when pulling c11926b on gerritholl:fix-fci-projection into d12bb7a on pytroll:master.

codecov · 2019-07-04T14:50:45Z

Codecov Report

Merging #845 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #845   +/-   ##
=======================================
  Coverage   89.54%   89.54%           
=======================================
  Files         200      200           
  Lines       29392    29392           
=======================================
  Hits        26319    26319           
  Misses       3073     3073

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

gerritholl · 2019-07-04T14:56:38Z

It looks like it's farther off the farther east.

gerritholl · 2019-07-04T15:17:32Z

The coverage decreased because I shortened some methods that were covered, but did not shorten any methods that weren't. I produced a negative number of LOC, so to say...

gerritholl · 2019-07-04T15:32:17Z

The offset is location-dependent.

Socotra (12°N, 54°E), too far south:

La Gomera and Tenerife (28°N, 17°W), too far west:

Bornholm (55°N, 15°E) too far east:

Île Europa (22°S, 40°E) too far north:

São Tomé (0°N, 7°E), almost correct:

There's still something wrong with either:

The projection information
The area extent information
The simulation of FCI from SEVIRI
The calculation of the rectified image from the swath
Something else

mraspaud · 2019-07-04T16:24:16Z

Maybe time to ask the data provider ?

gerritholl · 2019-07-08T15:48:11Z

Can this be due to different datums / projections / ellipsoids?

gerritholl · 2019-07-15T08:48:43Z

After discussion with EUMETSAT, we think the remaining projection issues are probably in the data and that the reader is fine.

Therefore, I propose this PR is ready to merge. Is anyone available to review it?

mraspaud

LGTM

mraspaud · 2019-10-21T16:11:04Z

Sorry for reviewing this so late, it passed under my radar.
I can merge this as it is, I was just wondering if you had tested with the latest test dataset ?

gerritholl · 2019-11-06T11:42:12Z

NB: EUMETSAT reports that with the same test data, they consider the geolocation to be accurate. So there is still something either with the reader, or with the coastline database.

mraspaud · 2019-11-06T11:47:54Z

Ok, interesting. Maybe something to look at during the next PCW ?

The scale factors and add offsets for the MTG FCI test data as published in 2019-09 are factually incorrect. This probably causes the differences between the pytroll and FCI geolocations. Until this is fixed from EUMETSATs side, hardcode corrected add_offset and scale_factor values. Also correct for an off-by-one error in the scale factors.

The scale factors and add offsets for the MTG FCI test data as published in 2019-09 are factually incorrect. This probably causes the differences between the pytroll and FCI geolocations. Until this is fixed from EUMETSATs side, hardcode corrected add_offset and scale_factor values. Similarly, there was a small error in the semi minor axis. Also correct for an off-by-one error in the scale factors.

gerritholl · 2019-12-19T10:46:05Z

Mean distance is now approximately 18 metre, after re-hardcoding several figures that were wrong in the file.

Which unfortunately defeats the title of this PR.

The FCI semi-minor axis correction does not improve anything, remove this hardcoding correction.

gerritholl · 2019-12-19T14:28:43Z

Those parameters will be corrected in the next version of the testdata, so the hardcoding won't be necessary. The median difference is down to 8.56 metre. That doesn't explain the coastline problem but I'm probably doing something wrong there independently of the reader.

gerritholl · 2019-12-19T14:40:33Z

Latest coastline comparison at Tenerife and La Gomera.

For comparison, for SEVIRI

gerritholl · 2019-12-19T14:59:57Z

Image with enhanced brightness (convert -brightness-contrast 50x50 in.tiff out.jpg)

FCI with latest geolocation corrections:

SEVIRI:

Swapping between the two, the island moves.

gerritholl · 2019-12-19T15:01:19Z

Are those differences consistent with what one would expect due to this being test data?

gerritholl · 2020-01-14T12:35:58Z

Related? SEVIRI georeferencing offset correction.

… fix-fci-projection

gerritholl · 2020-01-14T15:34:36Z

This is very likely caused by the SEVIRI georeferencing offset correction. Since SEVIRI data from before 2017-12 needs a correction, so does FCI test data based on SEVIRI data from before 2017-12. Thanks to @ameraner for finding this.

It looks like each of the SEVIRI readers repeat this correction:

seviri_l1b_hrit:

satpy/satpy/readers/seviri_l1b_hrit.py

Lines 586 to 589 in db1cc01

    
           xadj = 1500 
        
           yadj = -1500 
        
           aex = (aex[0] + xadj, aex[1] + yadj, 
        
                  aex[2] + xadj, aex[3] + yadj)

seviri_l1b_icare: not found

seviri_l1b_native:

satpy/satpy/readers/seviri_l1b_native.py

Lines 317 to 325 in db1cc01

    
           if earth_model == 2: 
        
               ns_offset = 0 
        
               we_offset = 0 
        
           elif earth_model == 1: 
        
               ns_offset = -0.5 
        
               we_offset = 0.5 
        
               if dataset_id.name == 'HRV': 
        
                   ns_offset = -1.5 
        
                   we_offset = 1.5

seviri_l1b_nc:

satpy/satpy/readers/seviri_l1b_nc.py

Lines 189 to 194 in db1cc01

    
           if earth_model == 2: 
        
               ns_offset = 0  # north +ve 
        
               we_offset = 0  # west +ve 
        
           elif earth_model == 1: 
        
               ns_offset = -0.5  # north +ve 
        
               we_offset = 0.5  # west +ve

Expand the mocking of variable attributes for the routine testing the FCI reader. One attribute was missing, and none were created in the "/attr/..." caching in which they are accessed by the reader.

ameraner · 2020-01-16T09:41:30Z

We can (finally) confirm that the root of the "big" shift is the geolocation offset present in SEVIRI images recorded prior to 2017, that went uncorrected into the creation of the FCI test data. When feeding the EUM processor with a SEVIRI image from 2019 as a proxy for the FCI data, and reading that FCI image with the FCI reader, the shift is gone:

mraspaud · 2020-01-16T09:44:47Z

@gerritholl @ameraner thanks for sorting this out and making the FCI reader perfectly geolocated.
@ameraner are there plans from EUM to provide an updated dataset that will take care of the aforementioned problems ?

Remove the hardcoded scale factor, add offset, and sweep. Those had been added to correct for errors in the 2019-09 release of FCI testdata, but should be fixed in the upcoming release, so should be no longer needed.

From the FCI reader, remove an unused hardcoded dictionary containing extent information. This information was only used for debugging, but is redundant relative to what is available in the file, assuming a full disk image is square.

In the FCI reader, make minor fixes to comments and docstring, and replace a RuntimeError by a more specific ValueError if an invalid calibration key is passed.

gerritholl · 2020-04-02T17:12:03Z

Using the 2020-04 pre-release testdata and satpy-0.20.1.dev211+ga8ddb7d3, which removes any hardcoded corrections for sweep, offset, or scale factor:

mraspaud

This looks good to me. I just have a couple minor comments. Thanks for the comprehensive work on the FCI reader!

satpy/readers/fci_l1c_fdhsi.py

mraspaud · 2020-04-02T21:59:14Z

satpy/readers/fci_l1c_fdhsi.py

-        # Channel dependent swath resoultion
-        area_extent = self.calc_area_extent(key)
+
+        # assumption: channels with same resolution should have same area


@ameraner can you confirm this ?

@mraspaud Yes it's correct, for this L1c FDHSI product we define two rectification grids (with 1 km and 2 km nadir resolution), and channels with the same resolution share the same grid (and therefore also same area definition).

perfect, thanks for then confirmation.

Apparently, PEP 257 insists that the closing quotes for a single-line docstring must be on the same line as the rest of the docstring. I will abide by what is apparently the community standard, although I will hereby note my (minority?) preference for the "closing quotes on next line" alternative. Co-Authored-By: Martin Raspaud <martin.raspaud@smhi.se>

Remove an empty line immediately after a method docstring, in order to satisfy the flake8-docstrings plugin.

ghost · 2020-04-03T09:26:04Z

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

👉 View analysis in DeepCode’s Dashboard

☺️ If you want to provide feedback on our bot, here is how to contact us.

gerritholl requested a review from mraspaud as a code owner July 4, 2019 09:34

gerritholl requested a review from djhoese as a code owner July 4, 2019 14:25

mraspaud approved these changes Oct 21, 2019

View reviewed changes

mraspaud assigned gerritholl Oct 21, 2019

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

gerritholl added this to In progress in PCW Copenhagen 2019 Nov 6, 2019

gerritholl added 2 commits December 19, 2019 11:04

Undo hardcoding FCI semi-minor axis

997a953

The FCI semi-minor axis correction does not improve anything, remove this hardcoding correction.

gerritholl added 3 commits January 14, 2020 15:58

Merge branch 'fix-fci-projection' of github.com:gerritholl/satpy into…

4689c99

… fix-fci-projection

Merge branch 'master' into fix-fci-projection

2e84f15

FCI reader flake8 fixes

8c96730

FCI, complement mocking of variable attributes

f537a14

Expand the mocking of variable attributes for the routine testing the FCI reader. One attribute was missing, and none were created in the "/attr/..." caching in which they are accessed by the reader.

gerritholl added 4 commits April 1, 2020 14:59

Merge branch 'master' into fix-fci-projection

9ddf4a1

FCI: Remove hardcoded parameters

fb0540e

Remove the hardcoded scale factor, add offset, and sweep. Those had been added to correct for errors in the 2019-09 release of FCI testdata, but should be fixed in the upcoming release, so should be no longer needed.

FCI: Remove unused hardcoded dictionary

e500e9d

From the FCI reader, remove an unused hardcoded dictionary containing extent information. This information was only used for debugging, but is redundant relative to what is available in the file, assuming a full disk image is square.

FCI: doc/comment fixes and change exception thrown

a8ddb7d

In the FCI reader, make minor fixes to comments and docstring, and replace a RuntimeError by a more specific ValueError if an invalid calibration key is passed.

gerritholl requested a review from mraspaud April 2, 2020 17:12

mraspaud reviewed Apr 2, 2020

View reviewed changes

gerritholl and others added 2 commits April 3, 2020 10:13

FCI: Remove empty line after method docstring

c11926b

Remove an empty line immediately after a method docstring, in order to satisfy the flake8-docstrings plugin.

mraspaud added this to the v0.21.0 milestone Apr 3, 2020

mraspaud merged commit 0b54987 into pytroll:master Apr 3, 2020

PCW Copenhagen 2019 automation moved this from In progress to Done Apr 3, 2020

gerritholl deleted the fix-fci-projection branch April 3, 2020 15:44

MTG: get projection and extent information from file #845

MTG: get projection and extent information from file #845

Conversation

gerritholl commented Jul 4, 2019 • edited by mraspaud

gerritholl commented Jul 4, 2019

gerritholl commented Jul 4, 2019

mraspaud commented Jul 4, 2019

mraspaud commented Jul 4, 2019

mraspaud commented Jul 4, 2019

gerritholl commented Jul 4, 2019

mraspaud commented Jul 4, 2019

gerritholl commented Jul 4, 2019

mraspaud commented Jul 4, 2019

gerritholl commented Jul 4, 2019

mraspaud commented Jul 4, 2019

gerritholl commented Jul 4, 2019

coveralls commented Jul 4, 2019 • edited

codecov bot commented Jul 4, 2019 • edited

Codecov Report

gerritholl commented Jul 4, 2019

gerritholl commented Jul 4, 2019

gerritholl commented Jul 4, 2019

mraspaud commented Jul 4, 2019

gerritholl commented Jul 8, 2019

gerritholl commented Jul 15, 2019

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud commented Oct 21, 2019

gerritholl commented Nov 6, 2019

mraspaud commented Nov 6, 2019

gerritholl commented Dec 19, 2019

gerritholl commented Dec 19, 2019

gerritholl commented Dec 19, 2019

gerritholl commented Dec 19, 2019

gerritholl commented Dec 19, 2019

gerritholl commented Jan 14, 2020

gerritholl commented Jan 14, 2020

ameraner commented Jan 16, 2020

mraspaud commented Jan 16, 2020

gerritholl commented Apr 2, 2020

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud Apr 2, 2020

Choose a reason for hiding this comment

ameraner Apr 3, 2020

Choose a reason for hiding this comment

mraspaud Apr 3, 2020

Choose a reason for hiding this comment

ghost commented Apr 3, 2020 • edited by ghost

👉 View analysis in DeepCode’s Dashboard

gerritholl commented Jul 4, 2019 •

edited by mraspaud

coveralls commented Jul 4, 2019 •

edited

codecov bot commented Jul 4, 2019 •

edited

ghost commented Apr 3, 2020 •

edited by ghost