Edge equality operator is incorrect (#333)

[nrkramer]

Demonstration that unique edges are "skipped" sometimes when adding to an EdgeSet_T. Wonder if it's hashing, equality, or user error?

Running this on my machine:

Which makes me believe we have a bug, are misunderstanding how std::unordered_set works, or I'm doing something silly.

Let me know what you guys think! @sbaldu @ZigRazor

Update

After much sleuthing, I've found the error to be innocuously hidden here.

It turns out our edge equality operator was incorrect. This causes the underlying hash bucket implemented by various STL incarnations to incorrectly match nodes that are otherwise proven unique (by our hashing function, but STL implementors sometimes check equality anyway).

(As an aside, maybe we should look at a better hash implementation. The bitwise xor ( ^ ) doesn't work for edges that use the same nodes (cycles) since the operation is mathematically symmetrical) - this can result in performance degradation in highly cyclical graphs. Copying boost::hash_combine seems like a good candidate)

[ghost]

👇 Click on the image for a new way to code review

Legend

[codecov]

Codecov Report

Merging #326 (1b4ed69) into master (7afc54e) will increase coverage by 0.00%.
The diff coverage is 97.72%.

@@           Coverage Diff           @@
##           master     #326   +/-   ##
=======================================
  Coverage   97.25%   97.25%           
=======================================
  Files          54       54           
  Lines        7934     7977   +43     
=======================================
+ Hits         7716     7758   +42     
- Misses        218      219    +1     

Flag	Coverage Δ
unittests	97.25% <97.72%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
include/Graph/Graph.hpp	94.87% <ø> (ø)
test/GraphTest.cpp	99.80% <97.67%> (-0.20%)	⬇️
include/Utility/PointerHash.hpp	100.00% <100.00%> (ø)

[sbaldu]

This is weird, I tried to reproduce this error but I haven't been able to do it. I copy pasted your exact code and I get all the 11 edges.
I also tried different ways of adding the edges to the graph and it always works fine:

  graph.addEdge(&edge1);

  CXXGraph::T_EdgeSet<int> edgeSet;
  edgeSet.insert(std::make_shared<CXXGraph::DirectedEdge<int>>(edge1));

  CXXGraph::T_EdgeSet<int> edgeSet;
  edgeSet2.insert(std::make_shared<CXXGraph::DirectedEdge<int>>(edge1.getId(), edge1.getFrom(), edge1.getTo()));

Also, in the picture that you provided the hash indexes of the edges are all different, so I doubt that the error that you get is due to the hash function.

[ZigRazor]

Ok, there is a BUG, the skipped nodes seems to be "new" to the graph. What compiler you are using?

[ZigRazor]

As said @sbaldu, but also as we can see in the CI/CD where the tests are executed, your test does not fail.
I'm wondering about the fact that with some compiler these code does not work.

[nrkramer]

What compiler you are using?

I'm using MSVC 14.36.32532 (v143)

Could it be that Microsoft has a bug in their std::unordered_set implementation? It seems unlikely, but possible...

[ZigRazor]

@nrkramer we can try to add in the CI/CD a workflow that works on windows and try to execute tests.
what do you think?

[ZigRazor]

@nrkramer I think is time to open a new Issue to track this.
Can you open a comphrensive issue with all the details and the analysis done until now?
Thank you in advance.

[ZigRazor]

@nrkramer please resolve conflit so we can merge this PR and open a new Issue.

[nrkramer]

@ZigRazor It looks like the MSVC builder has some issues.

I'm going sleep since it's super late where I am. Feel free to merge once the other checks have passed.