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

Iterator for voronoiedges #53

Closed
wants to merge 1 commit into from
Closed

Conversation

mikkoku
Copy link
Contributor

@mikkoku mikkoku commented Oct 16, 2020

Change voronoiedges to return an iterator instead of a Channel.

In my use case this results in a 10-100x faster iteration over voronoiedges.

@Wikunia
Copy link
Contributor

Wikunia commented Feb 16, 2022

I can confirm that a Channel seems extremely slow in my use case. What needs to be done to get this merged?
Is this project still maintained?

@dkarrasch
Copy link
Collaborator

Can someone give a little piece of code so that I can test for performance. Also, is this fully tested?

@Wikunia
Copy link
Contributor

Wikunia commented Feb 16, 2022

Thanks for the fast reply. Sure will create a small example for you. 😊 Regarding testing: I imagined the same tests should run that's why I'm a bit confused by the zero coverage on this PR but I'm happy to look into it. Just wanted to check first whether there is interested in this.

@dkarrasch
Copy link
Collaborator

Yes, if the interface doesn't change and we gain performance, there's interest, even though it hadn't looked like it since October 2020. Sorry for that. Maybe I'll need to update the test and coverage infrastructure on master, and then may ask @mikkoku to rebase. I'll get back to you ASAP.

@dkarrasch
Copy link
Collaborator

On master, this iterator seems to be untested, and so will be the modified version, see my #56. So I have to kind requests: first, could you please rewrite the "withoutgenerator" version in the same way, and second, could you please add some test, perhaps similar to the example in the README (i += 1, counting the edges, nothing fancy) for a small controllable example. That would make sure that the iterator goes through.

@Wikunia
Copy link
Contributor

Wikunia commented Feb 16, 2022

I created a fork https://github.com/Wikunia/VoronoiDelaunay.jl/tree/feature-iterator-instead-of-channel and branch there for this where you can checkout the test/current.jl file which creates n random points (like your readme) and then iterates over voronoi edges.

Here are two benchmark results for the iterate_over_tess_channel and iterate_over_tess_iterator functions for n = 100
Both result in i == 301 in the end.

julia> @benchmark iterate_over_tess_channel(tess)
BenchmarkTools.Trial: 3018 samples with 1 evaluation.
 Range (min … max):  1.480 ms …   4.860 ms  ┊ GC (min … max): 0.00% … 65.36%
 Time  (median):     1.647 ms               ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.653 ms ± 109.920 μs  ┊ GC (mean ± σ):  0.17% ±  2.00%
 Memory estimate: 48.70 KiB, allocs estimate: 626.


julia> @benchmark iterate_over_tess_iterator(tess)
BenchmarkTools.Trial: 10000 samples with 6 evaluations.
 Range (min … max):  5.285 μs …   8.555 μs  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     5.564 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   5.572 μs ± 139.950 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%
 Memory estimate: 0 bytes, allocs estimate: 0.

Which in this case is a speedup of about 300 and no allocations is of course always good :)

I can try to rewrite the without generator function in the same way but probably later today or tmr.
Shall I open a new PR here then?

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