Skip to content

Fix ancillary variable confusion after resampling #2336

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.

Sign up for GitHub

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

Merged
mraspaud merged 4 commits into from
Feb 2, 2023
Merged

Fix ancillary variable confusion after resampling #2336

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8fe2509
Added test for GH2329
gerritholl Dec 22, 2022
603552e
Use default keys if none defined
gerritholl Dec 22, 2022
793d76b
Merge branch 'main' into bugfix-resample-ancillary
gerritholl Feb 1, 2023
ad3ff76
Small rewrite in test_resample_multi_ancillary
gerritholl Feb 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions satpy/dataset/anc_vars.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
# satpy. If not, see <http://www.gnu.org/licenses/>.
"""Utilities for dealing with ancillary variables."""

from .dataid import DataID
from .dataid import DataID, default_id_keys_config


def dataset_walker(datasets):
Expand All @@ -39,7 +39,11 @@ def replace_anc(dataset, parent_dataset):
"""Replace *dataset* the *parent_dataset*'s `ancillary_variables` field."""
if parent_dataset is None:
return
id_keys = parent_dataset.attrs.get('_satpy_id_keys', dataset.attrs.get('_satpy_id_keys'))
id_keys = parent_dataset.attrs.get(
'_satpy_id_keys',
dataset.attrs.get(
'_satpy_id_keys',
default_id_keys_config))
current_dataid = DataID(id_keys, **dataset.attrs)
for idx, ds in enumerate(parent_dataset.attrs['ancillary_variables']):
if current_dataid == DataID(id_keys, **ds.attrs):
Expand Down
29 changes: 29 additions & 0 deletions satpy/tests/test_scene.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1796,6 +1796,35 @@ def test_resample_ancillary(self):
new_scene = scene.resample(dst_area)
assert new_scene['comp20'] is new_scene['comp19'].attrs['ancillary_variables'][0]

def test_resample_multi_ancillary(self):
"""Test that multiple ancillary variables are retained after resampling.

This test corresponds to GH#2329
"""
Comment on lines +1799 to +1803
Copy link
Member

@djhoese djhoese Feb 1, 2023

Choose a reason for hiding this comment

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

Are there other tests for resampling with ancillary variables? Or maybe just the data walker? I'm wondering if an existing test could be updated with the checks you include at the bottom of this test that would serve the same purpose as this test.

Copy link
Member Author

@gerritholl gerritholl Feb 1, 2023

Choose a reason for hiding this comment

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

There is test_resample_ancillary, which tests the case with a single ancillary variable. The bugs #2329 and #2330 occur only if there is more than one ancillary variable. I don't understand the data walker, but I don't think the tests involving it do anything with ancillary variables.

Copy link
Member

@djhoese djhoese Feb 1, 2023

Choose a reason for hiding this comment

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

I don't understand the data walker, but I don't think the tests involving it do anything with ancillary variables.

Well that's unfortunate then because that was the whole point of that function/generator iirc.

Copy link
Member Author

@gerritholl gerritholl Feb 1, 2023

Choose a reason for hiding this comment

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

There doesn't seem to be any test specifically for dataset_walker. The only mention in the test suite is in a mock:

$ find . -name '*.py' -exec grep dataset_walker {} +
./test_scene.py:                mock.patch('satpy.dataset.dataset_walker') as ds_walker:

from pyresample import create_area_def
sc = Scene()
n = 5
ar = create_area_def("a", 4087, resolution=1000, center=(0, 0), shape=(n, n))
sc["test"] = xr.DataArray(
np.arange(n*n).reshape(n, n),
dims=("y", "x"),
attrs={
"area": ar,
"name": "test",
"ancillary_variables": [
xr.DataArray(
np.arange(n*n).reshape(n, n)*i,
dims=("y", "x"),
attrs={
"name": f"anc{i:d}",
"area": ar})
for i in range(2)]})
subset = create_area_def("b", 4087, resolution=800, center=(0, 0),
shape=(n-1, n-1))
ls = sc.resample(subset)
assert ([av.attrs["name"] for av in sc["test"].attrs["ancillary_variables"]] ==
[av.attrs["name"] for av in ls["test"].attrs["ancillary_variables"]])

def test_resample_reduce_data(self):
"""Test that the Scene reducing data does not affect final output."""
from pyresample.geometry import AreaDefinition
Expand Down