KeRNeLith / QuikGraph

Generic Graph Data Structures and Algorithms for .NET
https://kernelith.github.io/QuikGraph/
Microsoft Public License
453 stars 65 forks source link

ArgumentException in UndirectedEdge constructor #32

Closed SirePi closed 3 years ago

SirePi commented 3 years ago

I'm probably missing something in the inner workings of the library, but I can't understand why there is the need to make sure that Source is "lower" (whatever that might mean, since we are working on arbitrary structures) than Target when constructing a new UndirectedEdge (and in fact there are specialized edge comparers that test if source == source && target == target OR source == target && target == source).

Would it work if I were to implement a CompareTo function that always returns 0?

KeRNeLith commented 3 years ago

Hello,

The point of having UndirectedEdge requiring Source to be "lower" than Target is because depending if edge implements the IUndirectedEdge interface, it will have an impact on which EdgeEqualityComparer is chosen to perform comparison by default.

You can have a look at this extension method.

You will notice that undirected edge is only comparing Source and Target equality (because they are sorted), where directed edge performs more checks. This is because an undirected edge represents both A -> B AND B -> A, whereas a directed edge is only A -> B OR B -> A. In order to efficiently check edge equality, vertices of an undirected edge are "sorted". To make sure this order constraint is respected Source must be "lower" than Target all the time.

So to answer your question about the implementation of a CompareTo that would always return 0, well this is not ideal because the contract is to make sure that a given vertex is always lower than another one. If all vertices come with a comparison of 0 then initializing an undirected edge with source = A and target = B, and another with source = B and target = A. You can notice that this situation will break the edge equality.

SirePi commented 3 years ago

Ah, thanks for the info. It's just strange to me that to define an undirected edge (which, as you said, represents both A > B and B > A), I have to be careful in constructing it with the vertices sorted.

I would have expected that, given this is a requirement of the edge, it would be the constructor's job to reverse source and target if they are not in the correct order.

Anyway, thanks again. It's clear now.

KeRNeLith commented 3 years ago

Well undirected edge is a quite specific structure because indeed it's not a directed edge. BTW you can use standard edge in most cases.

Of course the constructor of undirected edge would be able to sort inputs rather than throwing, but this was a behavior I conserved from original library. Note that I imagine the main reason behind this is because when you construct the object given a source and a target, then it can be a little bit disturbing if after construction those are not set the same way they were given. It like if you set a property and after the set the property is not set to the value you gave because of some internal treatments. That's not impossible nor forbidden, but just disturbing.

SirePi commented 3 years ago

Personally, I find it more disturbing to have to be careful about order in an undirected edge (and, even more disturbing, having Source and Target properties in an undirected structure), but that's my problem 😄

I understand it comes from the need to have a common structure between all edge types so, it's all good for me.

In any case I've already sorted it out (pun intended) on my side by using a static method that compares the two vertices and calls the constructor with parameters in the right order.

KeRNeLith commented 3 years ago

Glad to hear you find a way to make it working on your end ;-)