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

Address Mask Error with Rotated Datasets #1278

Merged
merged 3 commits into from Feb 21, 2018

Conversation

Projects
None yet
2 participants
@whlteXbread
Copy link
Contributor

whlteXbread commented Feb 16, 2018

This commit proposes a strawman fix for #1240 .

A test is included as well, and it uses new test fixtures to demonstrate the issue.

I believe this is the cleanest way to address the issue, though the transform round-trip is not particularly elegant.

Looking forward to hearing your thoughts!



def geometry_window(dataset, shapes, pad_x=0, pad_y=0, north_up=True,
pixel_precision=3):
rotated=False, pixel_precision=3):

This comment has been minimized.

@sgillies

sgillies Feb 20, 2018

Contributor

@whlteXbread what would you think about detecting the need to rotate from the dataset's transform within this function instead of in mask()? Passing rotated=False accidentally when the dataset is rotated will always give an erroneous result, no? And we might be able to prevent that.

This comment has been minimized.

@whlteXbread

whlteXbread Feb 20, 2018

Author Contributor

@sgillies I am happy to make this change!

I will point out, though, that the same logic holds for north_up. I'd be happy to move that in as well.

AFAICT, geometry_window is only called once in the library, but I understand if you wouldn't want to change the function signature at this point in time.

@sgillies
Copy link
Contributor

sgillies left a comment

@whlteXbread I have just one question and possibly a change that might be made. All your work is excellent, the tests in particular, and much appreciated!

@@ -74,10 +74,11 @@ def raster_geometry_mask(dataset, shapes, all_touched=False, invert=False,
pad_y = 0

north_up = dataset.transform.e <= 0
rotated = dataset.transform.b != 0 or dataset.transform.d != 0

This comment has been minimized.

@whlteXbread

whlteXbread Feb 20, 2018

Author Contributor

I do actually calculate whether rotated should be true or false based on the dataset transform, just like north_up. Both of these checks could easily be moved into geometry_window, however.

This comment has been minimized.

@sgillies

sgillies Feb 21, 2018

Contributor

Yes. Hmm, and good point about changing the API if north_up was moved. Let's leave this as it is. I'll approve and merge. Thanks again!

This comment has been minimized.

@whlteXbread

whlteXbread Feb 22, 2018

Author Contributor

Of course! Thanks for the review and the merge.

@sgillies sgillies merged commit f285de6 into mapbox:master Feb 21, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.06%) to 97.873%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment