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

Geos area reduction #115

Merged
merged 8 commits into from
May 3, 2018
Merged

Geos area reduction #115

merged 8 commits into from
May 3, 2018

Conversation

mraspaud
Copy link
Member

@mraspaud mraspaud commented Apr 29, 2018

Implement area-slicing for geos-based areas.

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

@@ -1369,6 +1370,127 @@ def proj4_string(self):
items = self.proj_dict.items()
return '+' + ' +'.join([t[0] + '=' + str(t[1]) for t in items])

def get_area_slices(self, area_to_cover):
"""Compute the slice to read from an *area* based on an *area_to_cover*."""
Copy link
Contributor

Choose a reason for hiding this comment

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

E501 line too long (83 > 79 characters)

# Intersection only required for two different projections
if area_to_cover.proj_dict['proj'] == self.proj_dict['proj']:
logger.debug('Projections for data and slice areas are'
' identical: {}'.format(area_to_cover.proj_dict['proj']))
Copy link
Contributor

Choose a reason for hiding this comment

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

E501 line too long (82 > 79 characters)

@coveralls
Copy link

coveralls commented Apr 29, 2018

Coverage Status

Coverage decreased (-0.1%) to 86.054% when pulling 01f3748 on feature-geos-area-reduction into 94f922c on develop.

@mraspaud mraspaud changed the title [WIP] Geos area reduction Geos area reduction Apr 29, 2018
@mraspaud mraspaud changed the base branch from master to develop April 29, 2018 09:35
@djhoese
Copy link
Member

djhoese commented Apr 29, 2018

Couldn't we use pyproj to transform X/Y coordinates between the source area and the crop area? Why do the 'geos' angles matter? Shouldn't we be able to use lon/lat as intermediate coordinates (or do the transform directly using pyproj's functions for doing that)?

Lastly, crop versus crop_around?

@mraspaud
Copy link
Member Author

I wrote this a long time ago, but iirc it was because I wanted to go directly from the instrument scanning angle to the lon/lat coordinates. But you're right, we could use proj and just divide the x and y projection coordinates by the altitude of the satellite to get the angle.

About crop_around vs crop, my idea was that just crop would be equivalent to __getitem__, and the around felt more intuitive name-wise when providing an area

@djhoese
Copy link
Member

djhoese commented Apr 29, 2018

I was thinking of not using satellite height at all. We should be able to use transform (https://jswhit.github.io/pyproj/pyproj-module.html#transform). This would only require the source PROJ.4, the target PROJ.4, and the extents of the two areas. We could convert from the crop area extents to the source area extents and use those to figure out the indexes of the area to crop. If any of the crop area extents can't be transformed (1e30 iirc) then we use the source area extent value for that coordinate.

I thought maybe crop could accept multiple keyword arguments crop(self, area=None, lonlat_bbox=None, xy_bbox=None): and then getitem would be for row/col index numbers.

@djhoese
Copy link
Member

djhoese commented Apr 29, 2018

Summary of conversation with @mraspaud on slack: The boundary objects already handle everything that would need to be handled if we used the X/Y PROJ.4 transforms. They also use spherical coordinates (I think) so it should work in all cases.


from trollsched.boundary import AreaDefBoundary, Boundary

data_boundary = Boundary(*get_geostationary_bounding_box(self))
Copy link
Member

Choose a reason for hiding this comment

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

How hard would it be to make this work for other projections? Couldn't you just use AreaDefBoundary if not geos?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid that every projection has it's own boundary to undefined/space pixels, eg in mollweide, polar stereographic. And to make this efficient, this boundary should be computed, vs eg search iteratively.

Copy link
Member

Choose a reason for hiding this comment

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

Then the use of AreaDefBoundary a couple lines below this shouldn't be allowed by that same reasoning, right?

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, if the ROI is geos then shouldn't that also use the above logic Boundary(*get_geostation_bounding_box(...))?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is true theoretically. However, I believe that users will mostly provide a well define ROI

@mraspaud mraspaud merged commit 2816628 into develop May 3, 2018
@mraspaud mraspaud deleted the feature-geos-area-reduction branch May 3, 2018 07:29
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.

4 participants