-
Notifications
You must be signed in to change notification settings - Fork 96
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 get_area_slices #117
Fix get_area_slices #117
Conversation
Codecov Report
@@ Coverage Diff @@
## master #117 +/- ##
=========================================
Coverage ? 87.25%
=========================================
Files ? 34
Lines ? 5510
Branches ? 0
=========================================
Hits ? 4808
Misses ? 702
Partials ? 0
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had one question that I commented inline. Another one though: how much work would it be to throw the boundary stuff in to pyresample in this PR?
logger.debug('Projections for data and slice areas are' | ||
' identical: %s', area_to_cover.proj_dict['proj']) | ||
# Get xy coordinates | ||
llx, lly, urx, ury = area_to_cover.area_extent | ||
x, y = self.get_xy_from_proj_coords([llx, urx], [lly, ury]) | ||
|
||
return slice(x[0], x[1] + 1), slice(y[1], y[0] + 1) | ||
xstart = 0 if x[0] is np.ma.masked else x[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are 'old style' area definitions the only ones that return a masked value? What do dask arrays return?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.get_xy_from_proj_coords
doesn't produce a dask array, but I suppose that would return np.nan ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time to test I guess 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is it not necessary since you are passing the extents which are scalars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, exactly
setup.py
Outdated
@@ -33,7 +33,7 @@ | |||
'quicklook': ['matplotlib', 'cartopy', 'pillow'], | |||
'dask': ['dask>=0.16.1']} | |||
|
|||
test_requires = [] | |||
test_requires = ['pytroll-schedule'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this maybe be added as an extra requirement too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking at integrating the boundary stuff really soon, so I don't know
rows = 1 | ||
cols = 1 | ||
target_y = target_y[data_slice[0]] | ||
target_x = target_x[data_slice[1]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this whole block of code got a lot smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the downside is that the arange is called before the slicing is performed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this be da.arange
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we would need to put dask as a requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, github's diff tool was making it look like this was the _dask
method.
git diff origin/develop **/*py | flake8 --diff
This fixes pytroll/satpy#218