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 helper method for checking areas in compositors #224

Merged
merged 5 commits into from Mar 16, 2018

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Mar 15, 2018

I made this PR early so that @mraspaud and I and others could discuss this because it may be controversial and I wanted to figure this out before I spent time implementing everything.

Right now this PRs solution for datasets with equal resolutions but different coordinates is to just remove the coordinates. @mraspaud voted to have it just choose the first datasets coordinates. I had already implemented what is in this PR currently so I didn't do this yet. My argument is that this may be a bad idea because coordinates for multiple composites may not be the same depending on what the input datasets were.

  • Tests added
  • Tests passed
  • Passes git diff origin/develop **/*py | flake8 --diff

@djhoese djhoese added this to the v0.9 milestone Mar 15, 2018
@djhoese djhoese self-assigned this Mar 15, 2018
@djhoese djhoese requested a review from mraspaud March 15, 2018 01:50
@@ -44,6 +44,7 @@
from satpy.utils import sunzen_corr_cos, atmospheric_path_length_correction
from satpy.writers import get_enhanced_image
from satpy import CHUNK_SIZE
from pyresample.geometry import AreaDefinition
Copy link
Contributor

Choose a reason for hiding this comment

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

F401 'pyresample.geometry.AreaDefinition' imported but unused

Copy link
Member

Choose a reason for hiding this comment

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

geeze, it's making comments too

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 59.985% when pulling 171bb27 on bugfix-check-areas into e6099da on develop.

@coveralls
Copy link

coveralls commented Mar 15, 2018

Coverage Status

Coverage increased (+0.6%) to 60.697% when pulling a323a52 on bugfix-check-areas into e6099da on develop.

@djhoese
Copy link
Member Author

djhoese commented Mar 15, 2018

I'm now leaning towards this always raising incompatible areas and never correcting.

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.

I think we should be more explicit about the dimension we work on here.

"mapped to the same area as the "
"low resolution bands. Must "
"resample first.")
datasets = self.check_areas(datasets + optional_datasets)
Copy link
Member

Choose a reason for hiding this comment

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

some compositors (palettecompositor) have a palette as optional dependency, and the palette has no area attribute, and has a different shape.

Copy link
Member

Choose a reason for hiding this comment

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

So maybe check that the dimensions are called x and y before going further ? (although I know we're not there yet)

Copy link
Member Author

Choose a reason for hiding this comment

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

Checking dimensions sounds good and makes sense. However this specific line is just for ratio sharpening so it should never be a palette.

@@ -382,6 +435,7 @@ def __call__(self, projectables, optional_datasets=None, **info):
sunalt, suna = get_alt_az(vis.attrs['start_time'], lons, lats)
suna = xu.rad2deg(suna)
sunz = sun_zenith_angle(vis.attrs['start_time'], lons, lats)
# FIXME: Make it daskified
Copy link
Member

Choose a reason for hiding this comment

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

an eays fix is to compute lons and lats

Copy link
Member Author

Choose a reason for hiding this comment

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

that's no fun

Copy link
Member

Choose a reason for hiding this comment

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

indeed

if len(data_arrays) == 1:
return data_arrays

if not all(x.shape == data_arrays[0].shape for x in data_arrays[1:]):
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we be checking only the x and y dimension sizes ? and what guarantees that all data array have the same number of dimensions ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. This was done assuming we were dealing with 2D reader datasets. Maybe I'm not completely xarray-minded yet.

@@ -44,6 +44,7 @@
from satpy.utils import sunzen_corr_cos, atmospheric_path_length_correction
from satpy.writers import get_enhanced_image
from satpy import CHUNK_SIZE
from pyresample.geometry import AreaDefinition
Copy link
Contributor

Choose a reason for hiding this comment

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

F401 'pyresample.geometry.AreaDefinition' imported but unused


def test_mult_ds_diff_dims(self):
"""Test that datasets with different dimensions still pass."""
from satpy.composites import CompositeBase, IncompatibleAreas
Copy link
Contributor

Choose a reason for hiding this comment

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

F401 'satpy.composites.IncompatibleAreas' imported but unused

@djhoese djhoese changed the title [WIP] Add helper method for checking areas in compositors Add helper method for checking areas in compositors Mar 15, 2018
@djhoese
Copy link
Member Author

djhoese commented Mar 15, 2018

Hm maybe this one isn't ready. I get an unexpected error about the time coordinates when making the simulated green band for ABI.

@djhoese djhoese merged commit 547819c into develop Mar 16, 2018
@djhoese djhoese deleted the bugfix-check-areas branch March 16, 2018 14:23
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.

None yet

4 participants