Fix issue #25#30
Open
goerch wants to merge 16 commits into
Open
Conversation
also fixing first of the performance problems.
| using Distances: euclidean | ||
|
|
||
| Coord = SVector{2,Int} | ||
| const Coord = SVector{2,Int} |
| view(grid, TL[1]:BR[1], TL[2]:BR[2]) .= Ref(first(sites)) | ||
| else | ||
| # Otherwise we check if all corners have the same closest site | ||
| # Otherwise we check if all corners have the same closest site ... |
Owner
There was a problem hiding this comment.
This part I don't understand
Collaborator
Author
There was a problem hiding this comment.
If you modify the code here like
if all(zip(mins, dists)) do ((min_dist, _), dist)
dist > min_dist
end
grid[TL[1]:BR[1], TL[2]:BR[2]] .= Ref(min_site)
return nothing
else
@show (distance, mins, dists)
# grid[TL[1]:BR[1], TL[2]:BR[2]] .= Ref(min_site)
return nothing
end
you'll see the non unique sites and if you also set the rectangle to the the site you'll see the test failures for Chebyshev distance. BTW we could also drop the whole code sequence, because filtering is good enough (see the benchmark for different find and filter combinations in legacy).
| function centre_anchor_condition(site, sites, TL, BR; distance=euclidean) | ||
| centre = @. TL + BR / 2 | ||
| anchor = find_closest_site(centre, sites) | ||
| anchor = find_closest_site(centre, sites; distance=distance) |
Owner
There was a problem hiding this comment.
Good find, didn't realise this was bugged
| return nothing | ||
| end | ||
|
|
||
| function unstable_partition!(f, A) |
Owner
There was a problem hiding this comment.
Can you benchmark the use of unstable_partition vs EarlyStopper? I'm up for switching because it's simpler, but performance is important.
Collaborator
Author
There was a problem hiding this comment.
Benchmark added. To improve the last test cases we probably need something like preset_voronoi from the legacy branch.
Results from my system with 1.8.2
(n, s) = (400, 20)
jfa_voronoi!
41.130 ms (0 allocations: 0 bytes)
dac_voronoi!
3.201 ms (0 allocations: 0 bytes)
redac_voronoi!
exact_aux = DiscreteVoronoi.exact_aux
1.085 ms (0 allocations: 0 bytes)
centre_anchor_aux = DiscreteVoronoi.centre_anchor_aux
1.056 ms (0 allocations: 0 bytes)
redac_voronoi_es!
exact_aux_es = DiscreteVoronoi.exact_aux_es
979.600 μs (0 allocations: 0 bytes)
centre_anchor_aux_es = DiscreteVoronoi.centre_anchor_aux_es
946.800 μs (0 allocations: 0 bytes)
(n, s) = (400, 400)
jfa_voronoi!
48.407 ms (0 allocations: 0 bytes)
dac_voronoi!
254.303 ms (0 allocations: 0 bytes)
redac_voronoi!
exact_aux = DiscreteVoronoi.exact_aux
7.985 ms (0 allocations: 0 bytes)
centre_anchor_aux = DiscreteVoronoi.centre_anchor_aux
6.368 ms (0 allocations: 0 bytes)
redac_voronoi_es!
exact_aux_es = DiscreteVoronoi.exact_aux_es
7.415 ms (0 allocations: 0 bytes)
centre_anchor_aux_es = DiscreteVoronoi.centre_anchor_aux_es
5.867 ms (0 allocations: 0 bytes)
(n, s) = (400, 8000)
jfa_voronoi!
64.304 ms (0 allocations: 0 bytes)
redac_voronoi!
exact_aux = DiscreteVoronoi.exact_aux
206.388 ms (0 allocations: 0 bytes)
centre_anchor_aux = DiscreteVoronoi.centre_anchor_aux
51.430 ms (0 allocations: 0 bytes)
redac_voronoi_es!
exact_aux_es = DiscreteVoronoi.exact_aux_es
199.030 ms (0 allocations: 0 bytes)
centre_anchor_aux_es = DiscreteVoronoi.centre_anchor_aux_es
50.399 ms (0 allocations: 0 bytes)
(n, s) = (400, 160000)
jfa_voronoi!
49.841 ms (0 allocations: 0 bytes)
redac_voronoi!
exact_aux = DiscreteVoronoi.exact_aux
7.645 s (0 allocations: 0 bytes)
centre_anchor_aux = DiscreteVoronoi.centre_anchor_aux
686.620 ms (0 allocations: 0 bytes)
redac_voronoi_es!
exact_aux_es = DiscreteVoronoi.exact_aux_es
7.466 s (0 allocations: 0 bytes)
centre_anchor_aux_es = DiscreteVoronoi.centre_anchor_aux_es
707.790 ms (0 allocations: 0 bytes)
Collaborator
Author
There was a problem hiding this comment.
One more remark: to replace _findmin with Base.findmin, EarlyStopper needs to support a keys method. Do you know how to implement it?
Collaborator
Author
|
My current benchmark numbers look fine now: We could include |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.