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

Add simple lon/lat based MODIS interpolation #31

Merged
merged 15 commits into from
Sep 10, 2021

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Sep 7, 2021

This ports the CSPP Polar2Grid MODIS lon/lat interpolation which only depends on longitude and latitude and is still fast.

  • Closes #xxxx
  • Tests added
  • Tests passed
  • Passes git diff origin/main **/*py | flake8 --diff
  • Fully documented

@djhoese djhoese self-assigned this Sep 7, 2021
@codecov
Copy link

codecov bot commented Sep 7, 2021

Codecov Report

Merging #31 (fe2d8e3) into main (47338f9) will increase coverage by 1.51%.
The diff coverage is 93.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
+ Coverage   78.02%   79.54%   +1.51%     
==========================================
  Files          16       19       +3     
  Lines        1156     1305     +149     
==========================================
+ Hits          902     1038     +136     
- Misses        254      267      +13     
Flag Coverage Δ
unittests 79.54% <93.62%> (+1.51%) ⬆️

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

Impacted Files Coverage Δ
geotiepoints/tests/utils.py 54.54% <54.54%> (ø)
geotiepoints/geointerpolator.py 91.66% <92.59%> (-3.08%) ⬇️
geotiepoints/simple_modis_interpolator.py 94.11% <94.11%> (ø)
geotiepoints/interpolator.py 78.44% <100.00%> (-1.22%) ⬇️
geotiepoints/modisinterpolator.py 98.75% <100.00%> (-0.13%) ⬇️
...otiepoints/tests/test_simple_modis_interpolator.py 100.00% <100.00%> (ø)

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 47338f9...fe2d8e3. Read the comment docs.

@djhoese
Copy link
Member Author

djhoese commented Sep 7, 2021

Do I need to make this an interpolator class?

@coveralls
Copy link

coveralls commented Sep 7, 2021

Coverage Status

Coverage remained the same at 0.0% when pulling fe2d8e3 on djhoese:feature-simple-modis into 47338f9 on pytroll:main.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Nice works I just have a bunch of questions!

geotiepoints/geointerpolator.py Outdated Show resolved Hide resolved
geotiepoints/simple_modis_interpolator.py Show resolved Hide resolved
geotiepoints/simple_modis_interpolator.py Show resolved Hide resolved
geotiepoints/simple_modis_interpolator.py Show resolved Hide resolved
geotiepoints/tests/test_simple_modis_interpolator.py Outdated Show resolved Hide resolved
@djhoese
Copy link
Member Author

djhoese commented Sep 10, 2021

Comparison Plot

geotiepoints_interp_comparison

lon_min, lon_max, lat_min, lat_max = (-137.9722, -33.380066, 60.134193, 84.827736)

Dask Profile - Angles - One chunk

image

Dask Profile - Simple - One chunk

image

@djhoese
Copy link
Member Author

djhoese commented Sep 10, 2021

Interestingly if I set the chunk size of the input to (800, -1) then I get the following results:

Dask Profile - Angle - 800 Chunk Size

image

Dask Profile - Simple - 800 Chunk Size

image

So the angle-based interpolation takes longer but uses a little less memory, but the simple interp (which uses map_blocks) takes less time and uses less memory. However, I have to acknowledge that 800 for a chunk size is not usually what we use and may be unrealistic. 800 rows * 1354 cols * 4 bytes (float32) -> ~4.3MB...pretty small chunk size.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Some final styling comments...


if res_factor == 4:
# Use linear extrapolation for the first two 250 meter pixels along track
m = (result_array[k0 + 5, :] - result_array[k0 + 2, :]) / (y[5, 0] - y[2, 0])
Copy link
Member

Choose a reason for hiding this comment

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

These for lines are duplicated, could they be factorized?

Copy link
Member Author

Choose a reason for hiding this comment

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

They aren't exactly repeated. They have different indexes. Given that there isn't a difference in number of lines (unless I'm reading it wrong) I think I'd prefer to keep it this way since it is more clear, at least to me, what the logic is trying to do. I'd be worried that putting things in a separate function would make it harder to parse this out.

Copy link
Member

Choose a reason for hiding this comment

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

The computation of m and b (slope and offset) is identical with an offset of 32, right? I think they should be factorized. Maybe even with the 500m, just passing the indices.

else:
# 500m
# Use linear extrapolation for the first two 250 meter pixels along track
m = (result_array[k0 + 2, :] - result_array[k0 + 1, :]) / (y[2, 0] - y[1, 0])
Copy link
Member

Choose a reason for hiding this comment

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

These three lines are duplicated too.

return new_lons.astype(lon_array.dtype), new_lats.astype(lon_array.dtype)


# def interpolate_geolocation(nav_array):
Copy link
Member

Choose a reason for hiding this comment

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

Is this commented function necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. I could have sworn I deleted this already. Removed it now anyway.

@djhoese djhoese merged commit b3ae790 into pytroll:main Sep 10, 2021
@djhoese djhoese deleted the feature-simple-modis branch September 10, 2021 20:48
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.

3 participants