Skip to content

Use arrays rather than Dicts for Contour Chasing #51

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.

Sign up for GitHub

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

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
+126 −90
Draft

Use arrays rather than Dicts for Contour Chasing #51

Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8c8c5c5
Implement performance improvements using bit masking for crossing cal…
sjkelly Jan 28, 2020
4be7ce2
wip
sjkelly Feb 14, 2020
631f177
pass tests
sjkelly Feb 14, 2020
3903806
store last position to do scan in linear time
sjkelly Feb 14, 2020
260fbaa
get that microsecond unit
sjkelly Feb 14, 2020
b1a9150
reuse cell array
sjkelly Feb 14, 2020
6e2b878
use a LUT for starting edge calc
sjkelly Feb 14, 2020
4e04b3f
Revert "use a LUT for starting edge calc"
sjkelly Feb 14, 2020
e1bb0fc
Revert "reuse cell array"
sjkelly Feb 14, 2020
d0607b8
use inbounds in a few spots
sjkelly Feb 14, 2020
daef760
use some more inbounds
sjkelly Feb 15, 2020
59bc3bc
reuse cell array
sjkelly Feb 14, 2020
d9ee21a
move to collapse branches
sjkelly Feb 15, 2020
e4a3aa2
cleanup logic in get next edge
sjkelly Mar 1, 2020
abed388
some more logic cleanups
sjkelly Mar 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions src/Contour.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,13 @@ function contour(x, y, z, level::Number)
trace_contour(x, y, z, level, cells, cell_pop)
end

function contour(cells, x, y, z, level::Number)
# Todo: size checking on x,y,z
cell_pop = get_level_cells!(cells, z, level)
trace_contour(x, y, z, level, cells, cell_pop)
end


"""
`contours` returns a set of isolines.

Expand All @@ -77,7 +84,10 @@ contours(::Any...) = error("This method exists only for documentation purposes")
`contours(x,y,z,levels)` Trace the contour levels indicated by the `levels`
argument.
"""
contours(x, y, z, levels) = ContourCollection([contour(x, y, z, l) for l in levels])
function contours(x, y, z, levels)
cells = cell_matrix(z)
ContourCollection([contour(cells, x, y, z, l) for l in levels])
end

"""
`contours(x,y,z,Nlevels::Integer)` Trace `Nlevels` contour levels at heights
Expand Down Expand Up @@ -187,6 +197,7 @@ end

# Maps cell type to crossing types for non-ambiguous cells
const edge_LUT = (SW, SE, EW, NE, 0x0, NS, NW, NW, NS, 0x0, NE, EW, SE, SW)
const start_edge_LUT = (0x00, 0x00, 0x01, 0x00, 0x01, 0x02, 0x00, 0x00, 0x01, 0x02, 0x00, 0x04, 0x00, 0x00)
Copy link
Contributor

@tomasaschan tomasaschan Feb 17, 2020

Choose a reason for hiding this comment

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

I think these should probably be refactored to use some name constants instead of just magic numbers.

Copy link
Member Author

@sjkelly sjkelly Feb 17, 2020

Choose a reason for hiding this comment

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

Thank you for looking at this. I agree. Even better, on the latest diff these aren't there at all.

I was playing around with collapsing the branching logic but it turns out that the codegen was the same since Julia inlined and LLVM unrolled the loops and merged everything.

Copy link
Contributor

@tomasaschan tomasaschan Feb 17, 2020

Choose a reason for hiding this comment

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

Yeah, I haven't actually spent any time with this codebase for several years, so someone else should probably review it too. That's why I left it as a flat comment, without either 👍 or 👎 - I want to leave the actual decision to someone else... :)


function _get_case(z, h)
case = z[1] > h ? 0x01 : 0x00
Expand All @@ -196,10 +207,19 @@ function _get_case(z, h)
case
end

function get_level_cells(z, h::Number)

function cell_matrix(z)
xi_max, yi_max = size(z)
cells = zeros(UInt8, (xi_max-1,yi_max-1))
end

function get_level_cells(z,h::Number)
cells = cell_matrix(z)
cell_pop = get_level_cells!(cells, z, h)
cells, cell_pop
end

function get_level_cells!(cells, z, h::Number)
xi_max, yi_max = size(z)

cell_pop = 0

Expand Down Expand Up @@ -235,12 +255,12 @@ function get_level_cells(z, h::Number)
end
end

return cells, cell_pop
return cell_pop
end

function findfirst_cell(m, from_x, from_y)
s = size(m)
for xi = from_x:s[1], yi = from_y:s[2]
@inbounds for xi = from_x:s[1], yi = from_y:s[2]
!iszero(m[xi,yi]) && return xi,yi
end
return 0,0
Expand Down Expand Up @@ -354,7 +374,7 @@ function trace_contour(x, y, z, h::Number, cells::Array, cell_pop)
nonempty_cells = cell_pop
(xi_0, yi_0) = (1,1)

while nonempty_cells > 0
@inbounds while nonempty_cells > 0
contour = Curve2(promote_type(map(eltype, (x, y, z))...))

# Pick initial box
Expand All @@ -365,13 +385,7 @@ function trace_contour(x, y, z, h::Number, cells::Array, cell_pop)

# Pick a starting edge
crossing = get_first_crossing(cell)
starting_edge = UInt8(0)
for edge in (N, S, E, W)
if !iszero(edge & crossing)
starting_edge = edge
break
end
end
starting_edge = start_edge_LUT[crossing]

# Add the contour entry location for cell (xi_0,yi_0)
add_vertex!(contour, interpolate(x, y, z, h, xi_0, yi_0, starting_edge), fwd)
Expand Down