Change A* output to a vector of tuples

[gdalle]

This PR solves #59 by making the output of A* independent from the underlying graph type.
Until now, a_star(g, ...) tried to reconstruct the shortest path as a Vector{E}, where E is the edge type of g. However, for labelled or weighted graphs, calling E(s, d) was not enough to create a valid edge, leading to a bug.

[gdalle]

For anyone interested, this is literally a 4-line review and can help a great deal!

[gdalle]

@paulmelis since you opened the initial issue, does this solve your problem?

[codecov]

Codecov Report

Merging #116 (716709c) into master (da30b3b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
- Coverage   97.54%   97.54%   -0.01%     
==========================================
  Files         109      109              
  Lines        6318     6317       -1     
==========================================
- Hits         6163     6162       -1     
  Misses        155      155              

[paulmelis]

Well, I just checked the original issue I created, but it seems LightGraphs is no longer in development, and my original example now fails with the latest versions (can't create weighted edges it seems).

[gdalle]

That's weird, it works on my side. Is this the snippet you ran?

using Graphs
using SimpleWeightedGraphs

g = SimpleWeightedDiGraph(3)
add_edge!(g, 1, 2, 0.5)
add_edge!(g, 2, 3, 0.8)
add_edge!(g, 1, 3, 2.0)

a_star(g, 1, 3)

[paulmelis]

Almost, I was doing using LightGraphs, not using Graphs. Perhaps that's the needed fix on my side

[gdalle]

Well the fix isn't there yet in Graphs.jl, but it's in the current PR, so if you want to test, you'll have to do

using Pkg
Pkg.add("SimpleWeightedGraphs")
Pkg.add(url="https://github.com/gdalle/Graphs.jl/", rev="shortest_paths")

[gdalle]

This is a breaking change and I'm not very confident with tags and releases, so I wouldn't mind if someone else took over the merging so I can see how it's done 😊

[matbesancon]

It would be better if we can make this a non-breaking change, for example by adding a new method rather than removing the previous one, or adding an optional argument indicating how to construct edges

[etiennedeg]

I agree, we should enforce in the interface for edges types the implementation of a constructor given only start and end points, which would set default values for weights, labels and other data.

[gdalle]

I agree, we should enforce in the interface for edges types the implementation of a constructor given only start and end points, which would set default values for weights, labels and other data.

I'm not sure about this. Until now, the Graphs.jl interface only specified things about the graphs themselves, not their edges, so that would be an even bigger change for many ecosystem packages, wouldn't it?

[etiennedeg]

I don't think it is that breaking to ask to implement a new method for edges. If users were using an edge type without implementing this type of constructor, A* would error anyway. It will be breaking only if we make some existing functions rely on the assumptions that these constructors must be implemented. Moreover, keeping the actual behavior would allow us to allow multigraphs without deprecating A*, if we would be willing one day to support them.

[gdalle]

On the other hand this would mean that for weighted edges, we ask the users to implement a method E(s,d) (in addition to E(s,d,w)), which would return an edge with weight 1. Isn't it a bit misleading to the user of a weighted graphs package if the path returned by A* contains only edges with (fake) weight 1?
@matbesancon @etiennedeg

[gdalle]

I would rather find a way to return a SimpleEdge whenever E(s,d) is not defined

[matbesancon]

I would rather find a way to return a SimpleEdge whenever E(s,d) is not defined

yes that sounds sensible

[matbesancon]

(::Type{E})(src::Integer, dst::Integer) where {E <: AbstractEdge} = SimpleEdge(src, dst)

That would set a default that can be overwritten

[etiennedeg]

To me, it feels like #13, if our graphs are not multigraphs, there is no reason to return edges, we could just return the sequence of vertices, and have an helper function that gives edges from the sequence of vertices. If we choose to be breaking, why not getting rid of edges ?

[gdalle]

If we choose to be breaking, that is what we should do. But the solution outlined by @matbesancon only changes the behavior when it errors anyway, so it is not breaking

[etiennedeg]

That would work but isn't it weird to return an object not of type E when calling E constructor?

[gdalle]

In any case it will be weird. But I think it would be weirder to return a path made of weighted edges with fake weights all equal to 1 for instance

[etiennedeg]

Ok, last try, could it be possible to ask for a graph g to implement edge(g, s, d), which return the edge between s and d ? We have has_edge(g, s, d), I'm intrigued that we have no method to gather this edge ?

[gdalle]

Ok, last try, could it be possible to ask for a graph g to implement edge(g, s, d), which return the edge between s and d ? We have has_edge(g, s, d), I'm intrigued that we have no method to gather this edge ?

@etiennedeg that would make sense to me, since it would allow returning a path that is made of native edges associated with the graph object. And the fix by @matbesancon would be a fallback that returns SimpleEdge instead.

[gdalle]

If that's okay with you, I'll get to it soon, and try to fix #120 in the process

[gdalle]

But wait isn't it breaking to ask users to implement an additional function, especially if we plan on using it elsewhere in the codebase?

[etiennedeg]

Yes, I think it will even be breaking for every graph implementation outside of Graph.jl. We could add a default function (as @matbesancon suggested) wich return a simple edge. It would be breaking only if we implement a method relying on the assumption that edge(g, s, d) must belong to edges(g).
We could also default to a function which browse the content of edges(g) until it find a matching edge (but then we throw performance away until the method is defined...). Probably not the best choice.
I think this kind of method could be very beneficial in the long way, maybe we can go with this first idea (default to simple edge), add a warning that graphs should implement this method, and in some future version of Graphs.jl, we would allow us to assume this method match the content of edges(g) (a bit like deprecating a function, but here we add a function to the interface)

[gdalle]

I would rather avoid defining edge(g, s, d) because if we don't control its spread it may end up being breaking without us noticing. So a local fix within A* seems safer for now. @matbesancon @etiennedeg should I move forward with Mathieu's suggestion?

[etiennedeg]

If we discard this solution, I think the only remaining reasonable suggestion is Mathieu's suggestion.

For the edge(g, s, d) solution (with the default implementation), it should not be particularly difficulty to control the spread, we just have to not use it in any already implemented method. If we fear to introduce breaking changes, we can also restrict us to not use it in any function, until a few releases where we can assume packages have updated their codes by reimplementing the new method.

Also something to take note: if we add a new method E(s,d) or edge(g, s, d) to the API, and that people start using these, there will be breaking changes as soon as people in other packages re-implement the edge method to return an edge of the correct type.

[gdalle]

I'll close this PR since it has a misleading name, and open a new one to fix the issue with @etiennedeg 's suggestion without adding it to the documented API. That way it is non-breaking and remains hidden. We can always go back to it at a later date, the important thing being that A* finally starts working on weighted graphs. Okay with you both ?
After thinking about @matbesancon 's suggestion a bit longer, having a constructor that doesn't return the right type is messed up ^^