Fix the copy constructor of the subgraph. #121
Conversation
|
We recently had a PR, #80, which touched on something close to this you might want to look at. Before this gets accepted, please add a test case which illustrates the problem, doesn't work on the past incorrect behavior, and works with your PR. |
|
I did not see the #80's relation with the copy ctor. Current copy ctor does not recursively copy the children and properties. I have added a test case in |
|
It was a quick thought. I've been a little preoccupied. |
kenkangxgwe
force-pushed
the
develop
branch
3 times, most recently
from
e068b32 to
f3f700a
Compare
| { | ||
| m_graph = x.m_graph; | ||
| m_graph = x.m_graph; | ||
| m_edge_counter = x.m_edge_counter; |
There was a problem hiding this comment.
If the subgraph is actually the entire graph keep count of its number of edges, else don't keep track of the member graph's number of edges? This seems like a change in function.
There was a problem hiding this comment.
If the m_graph is a child of its parent, we manually add all the vertices from x.m_graph to m_graph. This will automatically increase the m_edge_counter instead of assign x.m_edge_counter to it.
test/subgraph.cpp
Outdated
| // UNDER CONSTRUCTION | ||
|
|
||
|
|
||
| // This is a helper function to recusively compare two subgraphs. |
There was a problem hiding this comment.
Please add more detail about what it is testing, and the motivation for testing it.
| // This block generates a complete graph with 8 vertices, | ||
| // and puts the first and last four of the vertices into two children. | ||
| // Do these again to the children, so there are 4 grandchildren with 2 vertices for each. | ||
| // Use the copy constructor to generate a copy and compare with the original one. |
There was a problem hiding this comment.
What about subgraph edges? Those also need testing.
There was a problem hiding this comment.
I only checked the numbers. How do I compare two edge descriptors from two different graphs?
There was a problem hiding this comment.
You could load the structurally identical graphs with each node holding a unique ID in that graph identical to the equivalent edge in the other graph.
There was a problem hiding this comment.
Ok, I just added a comparison of any two corresponding edge indices.
kenkangxgwe
force-pushed
the
develop
branch
2 times, most recently
from
bfb7aac to
a397894
Compare
In the previous version, the children and their properties are not copied correctly. Please see the test case.
|
I'm going to try to revisit this Wednesday. I'm overloaded with work right now and can't give a review due diligence. |
|
Take your time and have a nice weekend |
|
@jzmaddock This PR looks good to me. It timed out on one test which could be rerun. |
|
I've pushed a fix for the timeout to develop .travis.yml - I suspect the PR will just timeout again if we re-run, so I'll go ahead and merge and we'll see. |
3 participants
In the previous version, the children and their properties are not copied correctly.