pyresample icon indicating copy to clipboard operation
pyresample copied to clipboard

Fix using cached LUTs in bilinear resampler

Open pnuu opened this issue 3 years ago • 10 comments

This PR fixes the bilinear resampling when the resampling LUTs have been cached.

The bug were introduced in #324 where a check was added to make sure the data array dimensions are in correct order. The test was done against an index array that only exists during the initial run, and a better comparison is against the shape of the source geo definition.

  • [ ] Tests added
  • [ ] Tests passed
  • [x] Passes git diff origin/main **/*py | flake8 --diff

pnuu avatar Jun 30 '22 07:06 pnuu

Codecov Report

Merging #438 (1f9f20d) into main (74eb088) will increase coverage by 0.07%. The diff coverage is 96.42%.

@@            Coverage Diff             @@
##             main     #438      +/-   ##
==========================================
+ Coverage   94.20%   94.28%   +0.07%     
==========================================
  Files          68       69       +1     
  Lines       12255    12388     +133     
==========================================
+ Hits        11545    11680     +135     
+ Misses        710      708       -2     
Flag Coverage Δ
unittests 94.28% <96.42%> (+0.07%) :arrow_up:

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

Impacted Files Coverage Δ
pyresample/ewa/ewa.py 75.86% <66.66%> (-0.93%) :arrow_down:
pyresample/bilinear/xarr.py 92.90% <100.00%> (ø)
pyresample/spherical.py 98.21% <100.00%> (+0.01%) :arrow_up:
pyresample/spherical_geometry.py 93.72% <100.00%> (ø)
pyresample/test/test_bilinear.py 100.00% <100.00%> (ø)
pyresample/test/test_geometry.py 99.51% <100.00%> (+0.02%) :arrow_up:
pyresample/_spatial_mp.py 82.94% <0.00%> (-0.63%) :arrow_down:
pyresample/geometry.py 87.17% <0.00%> (-0.11%) :arrow_down:
pyresample/geo_filter.py 100.00% <0.00%> (ø)
pyresample/utils/proj4.py 100.00% <0.00%> (ø)
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Jun 30 '22 07:06 codecov[bot]

In addition to Coveralls acting up, I can't get the tests to fail. I tried adding

_ = new_resampler.resample(self.data1)

to https://github.com/pytroll/pyresample/blob/main/pyresample/test/test_bilinear.py#L1198

As far as I understand, this test pretty much does what Satpy is doing, so it should be failing, but passes with the current main.

pnuu avatar Jun 30 '22 11:06 pnuu

Also res = new_resampler.get_sample_from_bil_info(self.data1) works. This is what Satpy is actually doing, while .resample() calls the .get_bil_info() method first.

pnuu avatar Jun 30 '22 11:06 pnuu

Yeah I would have expected adding get_sample_from_bil_info to the line in the test you specified to fail. My best guesses are:

  1. You are accidentally running a different version locally than you think you are (not main which should break).
  2. Something in satpy and/or the tests and/or the resampler itself is setting some attribute on the class itself so it "persists" when you make the new resampler in the test.

djhoese avatar Jun 30 '22 12:06 djhoese

I'm fairly certain it's the correct version because my import ipdb; ipdb.set_trace() in the test method is caught.

I modified the test so that it saves to a persistent location, then ran again with modification that skipped the saving. Still works fine.

pnuu avatar Jun 30 '22 12:06 pnuu

I have to finish my docstring updates for my antimeridian PR but then I'll switch branches and test on main.

djhoese avatar Jun 30 '22 12:06 djhoese

I also just noticed that I ran Ernst's tests again my antimeridian branch which is relatively old so it may not include things you may have fixed in your more recent PRs.

djhoese avatar Jun 30 '22 12:06 djhoese

I'm starting my holiday now, so I'll get back to this in August at the earliest.

pnuu avatar Jun 30 '22 12:06 pnuu

Ok, I've tracked down why the real world case fails and the test doesn't. It is this line of code:

https://github.com/pytroll/pyresample/blob/e531bd9c8bc42213ee128a09842797a1346dee06/pyresample/bilinear/xarr.py#L113

From what I understand this if block is only entered if there aren't neighbors for every output pixel (the source data doesn't fully cover the target area). In the test the target def being used is only 4x4 pixels and so res.size is equal to the size of the target area (16). So I think the tests need to be expanded for other coverages. I saw that there is another test area that doesn't overlap the input data, but I'm not sure that is "good enough". I'll try a quick test.

djhoese avatar Jun 30 '22 15:06 djhoese

Hhhmmm even the outside target area doesn't hit this condition. I'm not sure if it is a smart idea for me to dive further into this until you're back @pnuu. Let me know if you have ideas and I can try them out before then.

djhoese avatar Jun 30 '22 15:06 djhoese

Damn, had forgotten this. I'll see if I have time early next week to have a look.

pnuu avatar Sep 02 '22 11:09 pnuu

I tested with the target area that doesn't overlap the input area at all with the main branch code, and it works. And also enters this branch:

https://github.com/pytroll/pyresample/blob/e531bd9c8bc42213ee128a09842797a1346dee06/pyresample/bilinear/xarr.py#L113

Same with a partially overlapping target area (added with the latest commit to this PR) that gets a sample using pre-calculated LUTs.

pnuu avatar Sep 12 '22 11:09 pnuu

Looks like the test failure is an HTTP hiccup in the deploy test (no big deal) and a new failure in the EWA resampling. I guess I know what I'm working on this morning.

djhoese avatar Sep 12 '22 14:09 djhoese

Coverage Status

Coverage increased (+0.005%) to 94.13% when pulling 1f9f20d4b5366b123c45755e4ad65aa906df6019 on pnuu:bugfix-bilinear-cache-loading into df76244fded7011ef09269b921c6fee981319a92 on pytroll:main.

coveralls avatar Sep 12 '22 14:09 coveralls

I'm a little scared about the EWA failure. I didn't change anything and it seems to pass now. I guess I'll deal with it next time...

djhoese avatar Sep 12 '22 17:09 djhoese

I'm a little scared about the EWA failure. I didn't change anything and it seems to pass now. I guess I'll deal with it next time...

I was a bit surprised about the failure in the first place, as only thing I changed was the flake8 complaint. But yeah, merging.

pnuu avatar Sep 12 '22 17:09 pnuu