Fix MODIS cviirs-based interpolation #41

djhoese · 2022-05-25T16:18:10Z

Closes #39. Includes #40. Fixes a swath width issue in the cviirs-based interpolation. This PR also recreates the modis interpolation test data to use a less terrain-y dataset. This allows for tighter tolerances in the tests. The tests were also rewritten to use geodetic distance between actual and expected results which is an overall better test than pure value differences.

TODO: Going to rewrite the cviirs and simple tests to use the same utilities.

mraspaud

Looks good so far, the test data is bigger though, is this planned?

mraspaud · 2022-05-25T16:31:55Z

geotiepoints/tests/test_modisinterpolator.py

+    """
+    g = Geod(ellps="WGS84")
+    _, _, dist = g.inv(lons_actual, lats_actual, lons_desired, lats_desired)
+    print(dist.min(), dist.max())


Should this be here?

Nope. Left over for sanity checking. Thanks.

djhoese · 2022-05-25T16:55:28Z

The test file difference is very confusing. Both are compressed and therefore chunked (automatically done when compressing).

Here is the old 250m latitude:

        double lon_250m(phony_dim_2, phony_dim_3) ;
                lon_250m:_Storage = "chunked" ;
                lon_250m:_ChunkSizes = 5, 677 ;
                lon_250m:_Filter = "6,0,3,3385,1,8,0,0,1,0,0,0,0,0,0,0,0,0,0,0,0|1,9" ;
                lon_250m:_Endianness = "little" ;
                lon_250m:_NoFill = "true" ;

And new:

        float lat_250m(phony_dim_2, phony_dim_3) ;
                lat_250m:_Storage = "chunked" ;
                lat_250m:_ChunkSizes = 10, 677 ;
                lat_250m:_DeflateLevel = 9 ;
                lat_250m:_Endianness = "little" ;
                lat_250m:_NoFill = "true" ;

I'll have to do some research why the filtering is being added.

djhoese · 2022-05-25T17:06:05Z

There. Added the shuffle filter and now it is less than 10kb bigger.

…m differences on CI

djhoese · 2022-05-25T17:15:39Z

Well we have at least one case where running on my system the simple interpolation is <26m max distance, but on CI it is larger (1 pixel).

CI seems to compute things differently

codecov · 2022-05-25T18:09:37Z

Codecov Report

Merging #41 (05f21ca) into main (95e07ab) will decrease coverage by 0.33%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
- Coverage   80.15%   79.81%   -0.34%     
==========================================
  Files          19       18       -1     
  Lines        1300     1214      -86     
==========================================
- Hits         1042      969      -73     
+ Misses        258      245      -13

Flag	Coverage Δ
unittests	`79.81% <100.00%> (-0.34%)`	⬇️

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

Impacted Files	Coverage Δ
geotiepoints/modisinterpolator.py	`98.75% <100.00%> (-0.01%)`	⬇️
geotiepoints/simple_modis_interpolator.py	`94.44% <100.00%> (+0.32%)`	⬆️
geotiepoints/tests/test_modisinterpolator.py	`100.00% <100.00%> (ø)`
...otiepoints/tests/test_simple_modis_interpolator.py	`100.00% <100.00%> (ø)`
geotiepoints/version.py	`43.36% <0.00%> (-3.23%)`	⬇️
geotiepoints/__init__.py

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 95e07ab...05f21ca. Read the comment docs.

coveralls · 2022-05-25T18:19:26Z

Coverage increased (+78.9%) to 78.931% when pulling 05f21ca on djhoese:bugfix-cviirs-modis-testdata into 95e07ab on pytroll:main.

djhoese · 2022-05-25T18:32:57Z

Kind of hard to know how the coverage is changing when the coverage reporting was broken. I had it fixed in one of my other non-merged PRs but I've copied the changes here. @mraspaud I ran the tests locally with coverage reporting and modisinterpolator.py is 98% covered and simple is 94%. modisinterpolator.py doesn't have any tests where 5km interpolation is done and the number of coarse columns isn't provided (where it defaults to 271).

The 94% in the simple interpolation is for ImportError workarounds, a non-2D input error check, and the short circuit if the dask chunks are already scan-based. Some of this could be implemented, but I'm not concerned about it right now. I'd rather add these tests in some of my later PRs rather than worry about coverage in this PR.

mraspaud

Just a couple of minor comments, otherwise looks good to me.

mraspaud · 2022-05-30T13:37:51Z

geotiepoints/tests/test_modisinterpolator.py

+    """
+    g = Geod(ellps="WGS84")
+    _, _, dist = g.inv(lons_actual, lats_actual, lons_desired, lats_desired)
+    np.testing.assert_array_less(dist, max_distance_diff)  # meters


For clarity, I think we should provide a custom error message to assert_array_less.

err_msg=f"Coordinates are greater than {max_distance_diff} geodetic " "meters from the expected coordinates.")

Done.

mraspaud · 2022-05-30T13:38:46Z

geotiepoints/tests/test_modisinterpolator.py

+def test_cviirs_interp(input_func, exp_func, interp_func, dist_max):
+    lon1, lat1, satz1 = input_func()
+    lons_exp, lats_exp = exp_func()
+
+    # when working with dask arrays, we shouldn't compute anything
+    with dask.config.set(scheduler=CustomScheduler(0)):
+        lons, lats = interp_func(lon1, lat1, satz1)
+
+    if hasattr(lons, "compute"):
+        lons, lats = da.compute(lons, lats)
+    assert_geodetic_distance(lons, lats, lons_exp, lats_exp, dist_max)
+    assert not np.any(np.isnan(lons))
+    assert not np.any(np.isnan(lats))
+
+
+def test_cviirs_nan_handling():
+    # See GH #19
+    lon1, lat1, satz1 = load_1km_lonlat_satz_as_xarray_dask()
+    satz1 = _to_da(abs(np.linspace(-65.4, 65.4, 1354)).repeat(20).reshape(-1, 20).T)
+    lons, lats = modis_1km_to_500m(lon1, lat1, satz1)
+    assert not np.any(np.isnan(lons.compute()))
+    assert not np.any(np.isnan(lats.compute()))


I don't think we should use cviirs here, it might be cryptic for the unknowledgeable reader.

Changed it in these test names. Is that what you wanted?

djhoese · 2022-05-30T21:10:07Z

I think since I have commits here with multiple HDF5 test data versions we should squash this PR.

djhoese · 2022-05-31T13:28:32Z

Got slack approval. Merging.

djhoese added 6 commits May 24, 2022 15:08

Add right-most column "extrapolation" to simple modis interpolation

5b452b9

Update modis interpolation test data

b05d4ca

Fix scan width error in modis cviirs interpolation

8fe813b

Reduce tolerance of modis poles test

e5d7538

Use geodetic distance in tests

0de83ed

Switch modis interpolator to pytest

bf4ea02

djhoese added the bug label May 25, 2022

Move modis interpolation utilities to cviirs test module

5903208

mraspaud assigned djhoese May 25, 2022

Refactor modis interpolation tests to have data loading functions

c7be13f

mraspaud reviewed May 25, 2022

View reviewed changes

djhoese added 2 commits May 25, 2022 12:04

Refactor modis interpolation tests to use pytest parametrize

7825541

Add shuffle filter to modis test data creation

02594d7

djhoese requested a review from mraspaud May 25, 2022 17:06

djhoese added 2 commits May 25, 2022 12:09

Add missing pyproj test dependency

38bafcd

Make simple modis interpolation tests more lenient to deal with syste…

1c94ad8

…m differences on CI

djhoese added 3 commits May 25, 2022 12:23

Add verbose output to geodetic distance checks

9598c1c

Try weaker tolerances on modis interpolation tests

02c23ac

CI seems to compute things differently

Try weaker tolerances on modis interpolation tests

a5fa194

CI seems to compute things differently

Fix coverage reporting

729a100

djhoese added 3 commits May 25, 2022 13:34

Add missing toml dependency for coverage reporting

c6caf2d

Switch to temporary coveralls in CI that supports pyproject.toml

280395d

Remove unnecessary toml CI dependency

78417df

mraspaud reviewed May 30, 2022

View reviewed changes

Fixes based on reviewer comments

05f21ca

djhoese merged commit 3632ec7 into pytroll:main May 31, 2022

djhoese deleted the bugfix-cviirs-modis-testdata branch May 31, 2022 13:28

djhoese mentioned this pull request May 31, 2022

Add right-most column "extrapolation" to simple modis interpolation #40

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MODIS cviirs-based interpolation #41

Fix MODIS cviirs-based interpolation #41

djhoese commented May 25, 2022

mraspaud left a comment

mraspaud May 25, 2022

djhoese May 25, 2022

djhoese commented May 25, 2022

djhoese commented May 25, 2022

djhoese commented May 25, 2022

codecov bot commented May 25, 2022 •

edited

Loading

coveralls commented May 25, 2022 •

edited

Loading

djhoese commented May 25, 2022

mraspaud left a comment

mraspaud May 30, 2022

djhoese May 30, 2022

mraspaud May 30, 2022

djhoese May 30, 2022

djhoese commented May 30, 2022

djhoese commented May 31, 2022

Fix MODIS cviirs-based interpolation #41

Fix MODIS cviirs-based interpolation #41

Conversation

djhoese commented May 25, 2022

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud May 25, 2022

Choose a reason for hiding this comment

djhoese May 25, 2022

Choose a reason for hiding this comment

djhoese commented May 25, 2022

djhoese commented May 25, 2022

djhoese commented May 25, 2022

codecov bot commented May 25, 2022 • edited Loading

Codecov Report

coveralls commented May 25, 2022 • edited Loading

djhoese commented May 25, 2022

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud May 30, 2022

Choose a reason for hiding this comment

djhoese May 30, 2022

Choose a reason for hiding this comment

mraspaud May 30, 2022

Choose a reason for hiding this comment

djhoese May 30, 2022

Choose a reason for hiding this comment

djhoese commented May 30, 2022

djhoese commented May 31, 2022

codecov bot commented May 25, 2022 •

edited

Loading

coveralls commented May 25, 2022 •

edited

Loading