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

[sd/geometrybasics] Simplify Polygon code and add tests #57

Closed
wants to merge 5 commits into from

Conversation

greimel
Copy link

@greimel greimel commented Aug 4, 2021

The test cases are adapted from
this file in pyshp (commit e5407f38c571029721d7aaa9cabca2aa7ecd9019)

The main tests are shown below.

image

image

Right now the tests are in a Pluto notebook. If desired I can adapt the notebook to make it includable from runtests.jl (with or without the plots).

This addresses the first part of #39 (comment)

cc @SimonDanisch @Sov-trotter

@greimel greimel marked this pull request as draft August 4, 2021 14:17
@greimel greimel changed the title [sd/geometrybasics] Fix multi-polygon bug [sd/geometrybasics] Simplify Polygon code and add tests Aug 5, 2021
@greimel
Copy link
Author

greimel commented Aug 5, 2021

All tests pass locally. I have not yet integrated the new tests into the test suite. You will see that the old polygon tests yield a warning because they do not follow the official specification. (This is in line with pyshp: read correctly, but warn.)

I'll start using this branch now for some work, I'll report here if something needs to be fixed.

In the meantime, this is ready for review. Please let me know

  1. if you want me to include("notebook.jl") or just add the plain tests to the test file.
  2. if I should remove the old polygon tests (since they are incorrectly specified)

@greimel greimel marked this pull request as ready for review August 5, 2021 11:25
@rafaqz
Copy link
Member

rafaqz commented Dec 13, 2022

It would be good to use these tests, but the current (newer) implementation is lazy over rings which is much faster for many applications than separating out the polygons. I'm not sure this is possible with GeometryBasics.jl objects, so closing this PR.

@rafaqz rafaqz closed this Dec 13, 2022
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

2 participants