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

Check for GEOMETRY before extract / cast. #19

Merged
merged 4 commits into from
Mar 22, 2019
Merged

Check for GEOMETRY before extract / cast. #19

merged 4 commits into from
Mar 22, 2019

Conversation

dblodgett-usgs
Copy link
Contributor

Putting in place a check that avoids an expensive line of code that's not really needed in most cases. Related to #6.

This change shouldn't break anything -- the tests all pass. But I can't get the test data referenced in #6 to try it out. @mfherman, can you make some sample data available (without an API key)? I'll implement a test that hits that test and makes sure all's good.

@dblodgett-usgs
Copy link
Contributor Author

Don't merge this till we find some test data that hits this. Note that my original commit (which passed all the test) should have totally broken things but just made it so those lines don't get touched.

@mfherman
Copy link
Contributor

Here is the data referenced in #6 as downloaded from the Census Bureau API: areal_intersect_test.rda.zip

@dblodgett-usgs
Copy link
Contributor Author

🎊
Screen Shot 2019-03-21 at 9 28 57 PM

@dblodgett-usgs
Copy link
Contributor Author

Using your test data, we hit those lines of code again and the behavior will not have changed in the case that the geometry type is either GEOMETRY or GEOMETRYCOLLECTION.

Out of curiosity -- before hitting this code we get:

Browse[2]> unique(sf::st_geometry_type(intersection))
[1] POLYGON            LINESTRING         POINT              GEOMETRYCOLLECTION
[5] MULTIPOLYGON       MULTIPOINT      

The sf::st_collection_extract gives us:

Browse[2]> unique(sf::st_geometry_type(intersection))
[1] MULTIPOLYGON

and the call to sf::st_cast("MULTIPOLYGON") doesn't seem to do anything.

@chris-prener
Copy link
Owner

Really appreciate your attention on this @dblodgett-usgs! I just tested your code and like the patch a lot - this is a perfect solution.

@chris-prener
Copy link
Owner

for posterity's sake - AppVeyor failed because rlang was just updated - nothing to do with the areal package.

@chris-prener
Copy link
Owner

Also @mfherman - thanks for sending @dblodgett-usgs the data so he could test this!

@chris-prener chris-prener merged commit 76c1c69 into chris-prener:master Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants