search: fix handling of zero-weight cycle case

[kortschak]

• Allow FloydWarshall to write zero length cycles.
• Make Between excise zero-length cycles but mark as non-unique.
• Make AllBetween drop paths that extend into zero-length cycles.

This is option 2 in #72.

The recursive path reconstruction is a little subtle, but I think correct.

@vladimir-ch

[kortschak]

Actually, the Between code is incorrect. If you have any suggestions for an approach...

Now fixed, but obviously also subtle.

[vladimir-ch]

Is there anything that prevents (repeatedly) take the same zero-weight cycle? There may theoretically be many such cycles to take from a node but only one non-cycle path to advance the algorithm.

Here if such a cycle is detected, path is reverted, but on the next iteration from is advanced randomly to the next node on a shortest path which may lie on the same cycle. The path will be trimmed again but from will be already on the cycle. Is my understanding correct or am I missing something?

[kortschak]

Is there anything that prevents (repeatedly) take the same zero-weight cycle? There may theoretically be many such cycles to take from a node but only one non-cycle path to advance the algorithm.

I had something like that in the previous commit, and it was horribly incorrect. I'd be happy to see an approach that deals with this deterministically without excessive allocation. I can't think of one.

At the moment, I depend on probability. Also, zero weight cycles should be a reasonably rare case.

The path will be trimmed again but from will be already on the cycle. Is my understanding correct or am I missing something?

The path is not completely reverted, it is trimmed to one after the seen[from]. I have to say that I am quite unsure of this code, so if you can find a failing case for this, I'd be very grateful. I did many tests runs and I could not get any of the cases in the tests to fail.

[kortschak]

... adding this test still passes:

    {
        name: "zero-weight n·cycle directed",
        g:    func() graph.Mutable { return concrete.NewDirectedGraph() },
        edges: []concrete.WeightedEdge{
            // Add a path from 0->4 of weight 4
            {concrete.Edge{concrete.Node(0), concrete.Node(1)}, 1},
            {concrete.Edge{concrete.Node(1), concrete.Node(2)}, 1},
            {concrete.Edge{concrete.Node(2), concrete.Node(3)}, 1},
            {concrete.Edge{concrete.Node(3), concrete.Node(4)}, 1},

            // Add n zero-weight cycles.
            {concrete.Edge{concrete.Node(1), concrete.Node(5)}, 0},
            {concrete.Edge{concrete.Node(5), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(6)}, 0},
            {concrete.Edge{concrete.Node(6), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(7)}, 0},
            {concrete.Edge{concrete.Node(7), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(8)}, 0},
            {concrete.Edge{concrete.Node(8), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(9)}, 0},
            {concrete.Edge{concrete.Node(9), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(10)}, 0},
            {concrete.Edge{concrete.Node(10), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(11)}, 0},
            {concrete.Edge{concrete.Node(11), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(12)}, 0},
            {concrete.Edge{concrete.Node(12), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(13)}, 0},
            {concrete.Edge{concrete.Node(13), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(14)}, 0},
            {concrete.Edge{concrete.Node(14), concrete.Node(1)}, 0},
        },

        query:  concrete.Edge{concrete.Node(0), concrete.Node(4)},
        weight: 4,
        want: [][]int{
            {0, 1, 2, 3, 4},
        },
        unique: false,

        none: concrete.Edge{concrete.Node(4), concrete.Node(5)},
    },

[vladimir-ch]

zero weight cycles should be a reasonably rare case

I can imagine, that's why I wrote "theoretically".

The path is not completely reverted, it is trimmed to one after the seen[from]

Yes, that's what I meant. Sorry.

I think the code is correct. I originally thought that this repeated walk over zero-weight cycles could be avoided, but no, the correct path can stem from somewhere in the cycle. Negative weights seem to be quite a huge annoyance and the non-determinism in there is scary.

[vladimir-ch]

Yes, that's what I meant. Sorry.

In fact, that's not what I meant :-) I thought that the path is trimmed back to the beginning of the cycle, which was a ridiculous thought.

[kortschak]

I have a more general test for the non-productive path. I'll add that. It works with n=100, but is very slow with n=1000 (33s to complete cf. 0.005s for the n=10 case). If it becomes a problem, we can randomly permute the c slice and iterate through that slice, removing the chosen element. Can we leave this for a later PR when it is seen to be necessary?

[vladimir-ch]

Sure.

[kortschak]

I just had a go at it and it's not trivial. I think we actually need to keep a map[struct{from, to int}][]int and query that before calling p.at(from, to). If it's not present, copy from p.at(from, to) to the map at struct{from, to int}{from, to} and update the slice there. This means we only allocate when we are on a cycle and we don't disturb the internal representation.

[vladimir-ch]

LGTM