Skip to content

graph/labeled_graph: added remove_labeled_vertex - #354

Merged
jeremy-murphy merged 3 commits into
boostorg:developfrom
DeuceBox:remove-labeled-vertex
Nov 28, 2023
Merged

jeremy-murphy merged 3 commits into
boostorg:developfrom
DeuceBox:remove-labeled-vertex

Conversation

@DeuceBox

Copy link
Copy Markdown

I took this patch from this ticket:
https://svn.boost.org/trac/boost/ticket/9493
which no longer exists.

But I believe the original ticket issue was identical to #167 which this patch fixes.

@jeremy-murphy

Copy link
Copy Markdown
Collaborator

I'm looking on my phone so I might be wrong, but it looks like it's running GCC 4.X in the CI, which I thought I removed. Are you up to date on develop?

@DeuceBox
DeuceBox force-pushed the remove-labeled-vertex branch from de651cb to fa3bf25 Compare November 18, 2023 14:48
@DeuceBox
DeuceBox force-pushed the remove-labeled-vertex branch from fa3bf25 to 52c216c Compare November 18, 2023 22:34
@DeuceBox

Copy link
Copy Markdown
Author

I'm looking on my phone so I might be wrong, but it looks like it's running GCC 4.X in the CI, which I thought I removed. Are you up to date on develop?

You're correct that my branch was quite far behind develop. I just rebased on the latest develop, pushed again, and the tests were still failing. It appears that the original patch I preserved from the original svn ticket/tracker only accounted for unique associative containers, and not a random access / vector container that the tests use.

I did my best to update the patch and mimic the tag dispatch approach used by other methods to add support to remove_labeled_vertex for random access containers and multiple associative containers as well, and pushed that updated branch now,

@jeremy-murphy jeremy-murphy self-assigned this Nov 20, 2023
@jeremy-murphy

jeremy-murphy commented Nov 20, 2023 •

Copy link
Copy Markdown
Collaborator

Looks good. Could you please modify an existing unit test for labelled graphs to assert that the change is effective?

I.e., change a unit test so that it fails without this change, then put it together with your changes so that it passes.

PS. And if there is no existing unit test for labelled graphs... please write a really simple one. :)

@DeuceBox

Copy link
Copy Markdown
Author

I've added a test for removing a labeled vertex, based on the original #167 post's code snippet. I tested that it fails without my patch, and passes with my patch.

@jeremy-murphy

Copy link
Copy Markdown
Collaborator

I've added a test for removing a labeled vertex, based on the original #167 post's code snippet. I tested that it fails without my patch, and passes with my patch.

That's great.

It does make me wonder about duplicate labels, though -- what happens? Is it even possible? If duplicate labels are possible in any circumstance, then we need a test to confirm what happens.

@DeuceBox

Copy link
Copy Markdown
Author

Added a test that uses a multi associative container and adds two vertices with the same label, and then removes them one by one, asserting the graph vertex count at each stage.

@jeremy-murphy

Copy link
Copy Markdown
Collaborator

Added a test that uses a multi associative container and adds two vertices with the same label, and then removes them one by one, asserting the graph vertex count at each stage.

Fantastic. As soon as the CI is green, I'll merge it.

@jeremy-murphy
jeremy-murphy merged commit fa2636d into boostorg:develop Nov 28, 2023
@jeremy-murphy

Copy link
Copy Markdown
Collaborator

Thanks so much for fixing a long-standing bug!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants