JuliaGeometry / Contour.jl

Calculating contour curves for 2D scalar fields in Julia
Other
43 stars 13 forks source link

Provide a type-agnostic API and document it #29

Closed tomasaschan closed 8 years ago

tomasaschan commented 8 years ago

I got thinking about this when the lack of documentation was pointed out in #28. Currently, I think the way to e.g. plot all contour lines for a given surface xs,ys,zs is to do the following:

levels = contour(xs, ys, zs)
for level in levels
    for line in level.lines
        x,y = coordinates(line)
        # do something with the lists x and y, e.g. plot(x,y)
    end
end

I'd like this to become a little less dependent on the types we return; it would make the API a little more coherent, and it would make it easier to switch those types for something else, if we e.g. want to support higher dimensions. A suggestion for an API would be

c = contour(xs, ys, zs) # returns an object, not a list. Not necessary, but nice, IMO
for lvl in levels(c)
    for line in lines(lvl)
       x,y = coordinates(line)
       # plot(x,y) or whatever
    end
end

There are two changes here:

  1. A function lines which is basically lines(lvl) = lvl.lines. This should be uncontroversial.
  2. Changing the type of the output of contour to some type on which you can call levels and get todays result of contour. The natural name for such a type would be Contour, but that would require a rename (i.e. deprecation-and-replace) of this package. Although I think such a rename would be a good idea in the long run anyway, to adhere to Julia's package naming guidelines (see especially point 3.), it's no doubt a pretty disruptive change.

Thoughts?

cc: @darwindarak

darwindarak commented 8 years ago

:+1: Sounds like a good idea.

Maybe we can minimize the disruption of renaming the package by using two repositories for a release cycle. We can go ahead and create the Contours.jl package now, and make the current module throw out warnings when it is loaded.

SimonDanisch commented 8 years ago

In GeometryTypes we started using the decompose function for these kind of tasks. Maybe we should rename it to collect at some point, but it works in the following way:

points = decompose(Point, contour) # returns point coordinates of the line
lines = decompose(LineSegment, contour) # returns line segments
line = decompose(Line, contour) # returns a contigeous line
points = decompose(Point{3, Float32}, contour) # since we use types, we can also query types explicitely
points = decompose(Point{2, Float64}, contour) # return 2D points with Float64.
# this works for arbitrary geometry types as well:
points = decompose(Point, Pyramid(...))

This interface allows much flexibility while keeping the implementation very terse. It helps in plotting environments, were you already know what kind of data you need, independant of the kind of data you actually have (e.g. you have Contour{Float64}, but for plotting you know that you always need Point{3, Float32}). If you want to, you can use this as a low-level interface, and define higher level functions like lines on top of it.

tomasaschan commented 8 years ago

Hm. We currently depend on FixedSizeArrays, but I think it would be more appropriate to depend on GeometryTypes directly, and re-use as many as possible of the types there. I'll re-work #30 to this end.

tomasaschan commented 8 years ago

After looking at it again, it turns out that the relevant types from GeometryTypes are basically just re-exported from FixedSizeArrays. I won't go through the hassle, then :)

SimonDanisch commented 8 years ago

Haha yeah that's right. It would be rather about the LineSegment types and decompose ;) If you have a nice and simple line type lying around, you could also add it to GeometryTypes if you want.

tomasaschan commented 8 years ago

Hm. The LineSegment type could definitely provide a better abstraction over curves than our Curve2; is there a type (or typealias) in GeometryTypes representing a curve? Or is that just a Vector{LineSegment}?

SimonDanisch commented 8 years ago

Currently it is. But I think a curve should be another type, since linesegments are not joined... Or so I would think ;)

tomasaschan commented 8 years ago

Yeah, true. Then for the moment we should probably stick with our Curve2{T} (which is just a wrapper around a Vector{Point2{T}}) :smile:

SimonDanisch commented 8 years ago

Hehe, yeah I don't see why not!

tomasaschan commented 8 years ago

Now that #32 is merged, we can re-work the internal representation of everything without breaking the publicly documented API. Closing this.