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

Include non-convex polygon struct VPolygonNC ✳️ #3116

Merged
merged 16 commits into from
Oct 12, 2022

Conversation

mvanzulli
Copy link
Contributor

@mvanzulli mvanzulli commented Oct 7, 2022

The polygon interface aim to consider geometric figures like non-convex polygons in 2D in order to further on implement Jaccard and Haussdorf similarity numbers.

addressing #3115

Copy link
Member

@mforets mforets left a comment

Choose a reason for hiding this comment

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

I had an initial look.

src/Sets/Polygon.jl Outdated Show resolved Hide resolved
src/Sets/Polygon.jl Outdated Show resolved Hide resolved
src/Sets/Polygon.jl Outdated Show resolved Hide resolved
src/Sets/Polygon.jl Outdated Show resolved Hide resolved
@mvanzulli mvanzulli changed the title adding Polygon struct 🔳 Include non-convex polygon struct VPolygonNC 🔳 Oct 8, 2022
@mvanzulli mvanzulli changed the title Include non-convex polygon struct VPolygonNC 🔳 Include non-convex polygon struct VPolygonNC ✳️ Oct 8, 2022
@mvanzulli mvanzulli mentioned this pull request Oct 8, 2022
Copy link
Member

@schillic schillic left a comment

Choose a reason for hiding this comment

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

Maybe there should be a note about the order of the vertices in the documentation. For now we can allow both CW and CCW until we have a reason to choose, but you can also decide for one already.

Apart from that the PR looks good, thanks!
(@mforets can approve the build once ready.)

src/Sets/VPolygonNC.jl Outdated Show resolved Hide resolved
src/Sets/VPolygonNC.jl Outdated Show resolved Hide resolved
src/Sets/VPolygonNC.jl Outdated Show resolved Hide resolved
mvanzulli and others added 3 commits October 11, 2022 17:40
Co-authored-by: Christian Schilling <[email protected]>
Co-authored-by: Christian Schilling <[email protected]>
Co-authored-by: Christian Schilling <[email protected]>
@mvanzulli mvanzulli marked this pull request as ready for review October 11, 2022 21:52
@mforets
Copy link
Member

mforets commented Oct 12, 2022

In this repo the docs job fails if there are missing docstrings. Please create a new file VPolygonNC.md under docs/src/lib/sets.

@mvanzulli
Copy link
Contributor Author

In this repo the docs job fails if there are missing docstrings. Please create a new file VPolygonNC.md under docs/src/lib/sets.

Done

@mforets
Copy link
Member

mforets commented Oct 12, 2022

The docs job passed but there are some doctests which error >> https://github.com/JuliaReach/LazySets.jl/actions/runs/3233891985/jobs/5296713250#step:6:1206

@mvanzulli
Copy link
Contributor Author

mvanzulli commented Oct 12, 2022

The docs job passed but there are some doctests which error >> https://github.com/JuliaReach/LazySets.jl/actions/runs/3233891985/jobs/5296713250#step:6:1206

At least locally it is solved:

Test Summary:     | Pass  Total   Time
LazySets.doctests |    1      1  50.5s
 50.463812 seconds (77.29 M allocations: 5.399 GiB, 4.06% gc time, 58.11% compilation time: 1% of which was recompilation)
     Testing LazySets tests passed 

Copy link
Member

@mforets mforets left a comment

Choose a reason for hiding this comment

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

Tests passing!

I have a few minor requests listed below, but we can make a new issue to address them.

@mforets
Copy link
Member

mforets commented Oct 12, 2022

I have a few minor requests listed below, but we can make a new issue to address them.

See #3124

@schillic schillic merged commit 5022fec into JuliaReach:master Oct 12, 2022
@schillic
Copy link
Member

Thanks again for your contribution @mvanzulli! We just published a new release and I added your name to the release log. Let me know if you do not want to appear there.

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.

3 participants