-
Notifications
You must be signed in to change notification settings - Fork 81
Added vertex property inspectors. #107
base: master
Are you sure you want to change the base?
Conversation
Currently, a-star search is the only algorithm using any vertex properties (for the distance heuristic), so the a-star code was changed. The a-star code used the vertex type to index into the colormap, so would only work with graphs with integer vertex type. I generalized and abstracted away the colormap type. This probably should be a separate commit.
vertex properties can be passed through to the algorithm by an | ||
``VertexPropertyInspector``. An ``VertexPropertyInspector`` when | ||
passed to the ``vertex_property`` method along with an vertex and a | ||
graph, will return that property of an vertex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor English nitpick: it's "a vertex" not "an vertex" (only use "an" when the following word starts with a vowel sound).
vertex_property_requirement, vertex_property, | ||
|
||
AbstractVertexColormap, VectorVertexColormap, HashVertexColormap, | ||
vertex_colormap_requirement, setindex!, getindex, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as setindex!
and getindex
are imported from base, they don't need to be reexported.
(setindex!
does need to be imported above)
Thanks. I'm still learning the julia module system. (and the language in general) |
resource provided or required by that vertex as input. As the | ||
algorithms do not mandate any structure for the vertex types, these | ||
vertex properties can be passed through to the algorithm by an | ||
``VertexPropertyInspector``. An ``VertexPropertyInspector`` when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"An" on these two lines should also be "a".
No problem. You still need to import |
Three vertex property inspectors are provided | ||
``ConstantVertexPropertyInspector``, ``VectorVertexPropertyInspector`` and | ||
``AttributeVertexPropertyInspector``. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FunctionVertexPropertyInspector
should also be here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And AttributeVertexPropertyInspector
doesn't seem to be implemented? Or was one replaced with the other?
Do I really need to import Base.setindex! ? As I understand things, setindex! is an operator, and so should be implicitly imported. I'll fix the other export screwups. |
Nope, you're right! |
How about allowing the property inspectors to be constructed with something like const_inspector = vertex_inspector(g, 0)
color = zeros(num_vertices(g), Int)
color_inspector = vertex_inspector(g, color)
weight = Dict{ExVertex, Float64}()
weight_inspector = vertex_inspector(g, weight)
fn(v) = v.weight * 2.6
fn_inspector = vertex_inspector(g, fn) You could still have direct construction via the type constructors, but it would be nice to have a uniform (and short) interface that did the right thing most of the time. Good bad idea? |
I like @kmsquire's idea of concise inspector-construction functions. |
I too think it is a good idea. It is a bit more complicated, as the inspectors are also carriers of type information, so the last example would have to be something like
because functions have no type. If I have time tonight, I'll try to get to it. |
Function calls can't have types either, at least in fn(v) = v.weight * 2.6
fn_inspector = vertex_inspector(g, fn, Float64) When constructing the type, is knowing the graph useful? In your current PR, the graph is not included in the inspector (or constructors), but it could give the expected vertex and edge type: function vertex_inspector{V,E}(g::AbstractGraph{V,E}, fn::Function, rettype::Type)
...
end |
In my mind, we don't really have to introduce vertex_property(a, v, g) We can however introduce a series of fallback functions, so that people can simply grab a constant value or an array and supply it as a property inspector, as vertex_property{V}(a::AbstractArray, v::V, g::AbstractGraph{V}) = a[vertex_index(v, g)]
vertex_property{V}(a::Number, v::V, g::AbstractGraph{V}) = a Of course, people can implement more complex logics if they want. |
@lindahua We also need to implement |
FWIW, I concur with @lindahua that the interface can be much less strict. |
The AbstractPropertyInspector types are rather too Java-ish for my taste. |
Guilty as charged, I suppose. The mortgage company likes being paid:) The one other hitch with using generic objects is that we need to add a function to get the type returned. This is not difficult, but I have not done enough testing to know if it interferes with the compiler magic. This should not be a large problem, as we can pass the type in to the low level function that does the work. (We do this implicitly for Dijkstra and Bellman-Ford). My original reason for using inspector objects instead of the more natural functions was that the inspectors ended up being considerably faster. I'll try to get something done tonight. |
Ok, this backs out some controversial changes and implements @lindahua's inspectors. I am of two minds about this, the interface is cleaner, but it can generate some cryptic error messages for simple errors. |
abstract AbstractEdgePropertyInspector{T} | ||
#constant property | ||
vertex_property(x::Number, v, g) = x | ||
vertex_property_requirement(x, v, g) = none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none
=> nothing
(none
isn't a value in Julia)
Fixed speeling error.
…hem as examples in test. Still need to try documentation.
This last set of changes removes the |
Hi @deinst ,
I have proposed PR #184 to ensure that CP: @timholy |
Currently, a-star search is the only algorithm using any vertex properties (for
the distance heuristic), so the a-star code was changed.
The a-star code used the vertex type to index into the colormap, so would
only work with graphs with integer vertex type. I generalized and abstracted
away the colormap type. This probably should be a separate commit.
It may seem that I abstracted the colormap too far, but allowing the colormaps to be
hashtables when the vertex_map or edge_map is not available will allow us to use
hash based graphs a la networkx. Then we will be able to add and delete vertices and
edges, and the algorithms will only be a constant factor slower (assuming a good dict implementation).
The current implementation seems to be transparent after the compiler does its magic.
If this is OK, I'll make a hash based graph type (actually 2 because it makes sense to do directed and undirected
graphs separately.