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

Fix composites failing on non-aligned geolocation coordinates #2690

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Dec 14, 2023

See #2668 for detailed discussion. Basically #2610 switched from the internal satpy base resampler to the pyresample base resampler class. This overall isn't a problem except for the pyresample base resample has a shortcut where it won't process an input if it detects that the target geometry is the same as the source geometry. In those cases it returns the original DataArray with no additional modification. This logically/theoretically shouldn't be a problem, but it reveals a larger issue.

When y and x coordinates are generated from different sources they may have very small differences due to floating point precision issues. So example an x and y that are a subset/slice of a larger pair of x/y arrays may be different than an x/y generated from a sliced/subset AreaDefinition of the same original area definition (again, look at my comments in the referenced issue).

This PR updates one part of Satpy to make these operations work as overall the resampler change revealed an existing issue that was previously being hidden. The composite base class now performs a call to xarray.align to make sure that input arrays are compatible, but for y and x coordinates it will re-assign them to match the first input. The docstring in the new method explains any concerns about this.

@djhoese djhoese added bug component:resampling component:compositors cleanup Code cleanup but otherwise no change in functionality labels Dec 14, 2023
@djhoese djhoese self-assigned this Dec 14, 2023
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4076e99) 95.35% compared to head (cfbcaf7) 95.35%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2690   +/-   ##
=======================================
  Coverage   95.35%   95.35%           
=======================================
  Files         371      371           
  Lines       52400    52419   +19     
=======================================
+ Hits        49964    49983   +19     
  Misses       2436     2436           
Flag Coverage Δ
behaviourtests 4.16% <20.00%> (+<0.01%) ⬆️
unittests 95.97% <100.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7214139914

  • 30 of 30 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 95.917%

Totals Coverage Status
Change from base Build 7209861856: 0.001%
Covered Lines: 50109
Relevant Lines: 52242

💛 - Coveralls

@simonrp84
Copy link
Member

Adding a note here to say that this fixes the problem I have been experiencing with cropping Himawari data:

scn = Scene(ifiles_l15, reader='ahi_hsd')
scn.load(['true_color_reproduction'])
scn2 = scn.crop(xy_bbox = bbox)
scn3 = scn2.resample(scn2.coarsest_area(), resampler='native', reduce_data=False)
scn3.save_datasets(base_dir=tod, writer='simple_image')

The above code fails on satpy's main but runs correctly with this PR.

@yufeizhu600 Maybe you can try this PR with the problems you were having with @strandgren's geocolor PR - guess the two PRs can be used together...somehow?

@yufeizhu600
Copy link
Contributor

yufeizhu600 commented Dec 15, 2023

@yufeizhu600 Maybe you can try this PR with the problems you were having with @strandgren's geocolor PR - guess the two PRs can be used together...somehow?

@simonrp84 , you tagged wrong person. :). I think you meant to tag @yukaribbba.

@yukaribbba
Copy link
Contributor

@yufeizhu600 @simonrp84 Thanks. I'll have a try.

@simonrp84
Copy link
Member

Sorry for tagging the wrong person! Typed 'y' and went with githubs first suggestion 🙃 that's why you shouldn't code when tired,folks.

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.

LGTM! I think it's good to merge as it is, I'm just wondering if we should have an new method called eg unify_geo_coordinates that groups check_geolocation and align_geo_coordinates into one place?

Copy link
Member

@ameraner ameraner left a comment

Choose a reason for hiding this comment

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

Indeed the fix works, thanks for implementing a nice solution so quickly!

Unfortunately however, the NDVI-corrected True Color still fails; I took a look and it's because the NDVIHybridGreen calls match_data_arrays only on a subset of projectables, and then mixes these with the other unmatched projectables. This is fixed by

diff --git a/satpy/composites/spectral.py b/satpy/composites/spectral.py
index 448d7cb..ba417ac 100644
--- a/satpy/composites/spectral.py
+++ b/satpy/composites/spectral.py
@@ -160,9 +160,9 @@ class NDVIHybridGreen(SpectralBlender):
         LOG.info(f"Applying NDVI-weighted hybrid-green correction with limits [{self.limits[0]}, "
                  f"{self.limits[1]}] and strength {self.strength}.")

-        ndvi_input = self.match_data_arrays([projectables[1], projectables[2]])
+        ndvi_input = self.match_data_arrays(projectables)

-        ndvi = (ndvi_input[1] - ndvi_input[0]) / (ndvi_input[1] + ndvi_input[0])
+        ndvi = (ndvi_input[2] - ndvi_input[1]) / (ndvi_input[2] + ndvi_input[1])

if you want you can include this in this PR, or I can make a quick separate one.

@djhoese
Copy link
Member Author

djhoese commented Dec 15, 2023

I'm just wondering if we should have an new method called eg unify_geo_coordinates that groups check_geolocation and align_geo_coordinates into one place?

@mraspaud I had this same thought, but with drop_coordinates and align_geo_coordinates. So...maybe that's a sign that they should stay the way they are?

@mraspaud
Copy link
Member

I'm just wondering if we should have an new method called eg unify_geo_coordinates that groups check_geolocation and align_geo_coordinates into one place?

@mraspaud I had this same thought, but with drop_coordinates and align_geo_coordinates. So...maybe that's a sign that they should stay the way they are?

ok :)

@mraspaud mraspaud merged commit 6d652a6 into pytroll:main Dec 15, 2023
18 of 19 checks passed
@djhoese djhoese deleted the bugfix-aligned-coords branch December 15, 2023 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cleanup Code cleanup but otherwise no change in functionality component:compositors component:resampling
Projects
Development

Successfully merging this pull request may close these issues.

FCI HRFI true_color unavailable even after native resampling if upper_right_corner is used
7 participants