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

tests for counting edges #57

Merged
merged 4 commits into from
Feb 16, 2022

Conversation

Wikunia
Copy link
Contributor

@Wikunia Wikunia commented Feb 16, 2022

Before continuing with #53 I think it would be good to have these test cases.
I assumed that the number is correct here just wanted to add a test such that we see when this changes. Not sure how to better test this.

@dkarrasch
Copy link
Collaborator

Awesome. Could you please rebase onto current master, so that the github action CI stuff runs on this PR.

You could try and see if something like length(voronoiedges(tess)) works and is equal to the result of your loop. That should be an independent "stepping through" test. Could you please also fix the random seed in the "point can be in only one triangle" test. That test turned out to give spurious errors, probably when the test point was lying on some edge.

@dkarrasch
Copy link
Collaborator

With #53 we won't need the collect in the tests anymore, right?

@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #57 (235cf75) into master (63898ec) will increase coverage by 15.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #57       +/-   ##
===========================================
+ Coverage   72.20%   87.26%   +15.05%     
===========================================
  Files           1        1               
  Lines         421      424        +3     
===========================================
+ Hits          304      370       +66     
+ Misses        117       54       -63     
Impacted Files Coverage Δ
src/VoronoiDelaunay.jl 87.26% <0.00%> (+15.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63898ec...235cf75. Read the comment docs.

@Wikunia
Copy link
Contributor Author

Wikunia commented Feb 16, 2022

Yes that might be true though I'm not 100% how iterators handle length 😄

@Wikunia
Copy link
Contributor Author

Wikunia commented Feb 16, 2022

Seems like there is a difference in the delaunay edges between Julia 1.6 and 1.7 though? 😕

@dkarrasch
Copy link
Collaborator

Hm, I believe we've had changes in the random number generator between v1.6 and v1.7 or so. We should perhaps try to avoid hard-coding numbers, and instead compare two different implementations that we expect to give consistent results.

@Wikunia
Copy link
Contributor Author

Wikunia commented Feb 16, 2022

Oh yeah that's true maybe have a file with a list of points that we can read and use those as our test case?

@dkarrasch
Copy link
Collaborator

We don't need a big test. Perhaps you could manually type a couple of points, say 5, at distinct locations, and then we know everything about that specific case. It is only important to step through the iterator, even if its length is short.

@Wikunia
Copy link
Contributor Author

Wikunia commented Feb 16, 2022

True, will do

@Wikunia
Copy link
Contributor Author

Wikunia commented Feb 16, 2022

Some lines have a length of zero though:

image

Here you can see the 12 visible voronoi edges.

@Wikunia
Copy link
Contributor Author

Wikunia commented Feb 16, 2022

With #53 we won't need the collect in the tests anymore, right?

Actually we do because we don't provide the length of the iterator at the moment. We could walk through all and count and return that though if we want to.

@dkarrasch dkarrasch merged commit f484df6 into JuliaGeometry:master Feb 16, 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.

2 participants