feat: implement bidirectional adjacency graph

[jsjuni]

If you're comfortable with a separate vertices_dict for in-edges, I think this approach is cleaner. A DirectedAdjacencyGraph has hardly any state other than the vertices_dict, so why not simply make a separate graph? This avoids having to copy any implementation from the base class; it is implemented using only methods exposed by DirectedAdjacencyGraph.

This was my original proposed approach.

The only downside that I can see is that it duplicates the vertex references used as keys in the two vertices_dicts and duplicates operations on them. As you noted a couple of days ago, however, it is impossible to eliminate that without pulling some implementation into the subclass.

I restored the original module and made the changes to the test you suggested.

[monora]

I am fine with this implementation. Thank you! When we are through with my comments, I would prepare a new release with your changes.

[monora]

In #76 (comment) I meant to define two separate test classes:

1. Your new class
2. The test class which inherits from TestDirectedGraph

I think, that this would increase readability. Perhaps providing a comment, that we simply reuse the tests from the super test class.

[jsjuni]

I've been unable to make this work.

First of all, TestDirectedGraph has dependencies on DirectedAdjacencyGraph scattered throughout. It's not difficult to hoist them all up into #setup, but that would have to be done.

I tried it, but simply subclassing the TestDirectedGraph and overriding #setup did not work as expected. I'll push what I have but it's not right yet.

[monora]

For me that worked. See changes in 8c98f8a.

It also contains a minor doc change:

# This implementation of {BidirectionalGraph} creates an internal
  # {DirectedAdjacencyGraph} to store the in-edges and overrides methods

[jsjuni]

It appears to work but when you run the tests there's no result for TestBidirectionalAdjacencyGraphBasic; only the superclass TestDirectedGraph appears. I tried overriding setup in a way that all tests would fail and nothing failed. I don't understand this.

[monora]

You are right. I do not understand it either. The POLS is violated here by the test library. I just ran the test in TestDirectedGraph using the new BidirectionalAdjacencyGraph. All passed!

I propose to forget this experiment. I do not want to dig deeper...

[jsjuni]

Agreed.

[jsjuni]

Before you finalize anything, I think there's a bug in the constructor for BidirectionalAdjacencyGraph. I'm not sure it works if you pass it a graph argument.

[monora]

Ok, which tests do we need?

[jsjuni]

I fixed the constructor and added tests in both directed_graph_test.rb and bidirectional_graph_test.rb. I fixed test_reverse in both, which was modifying a graph used for multiple tests. I think it's good now.

[monora]

Close and release in #83

[monora]

@jsjuni : I reopened to let you push the contents of e70fdc0. This way you would be the author of the changes in master.

[jsjuni]

I'm not very experienced at working across forks. Can you clarify what you want me to do?

[monora]

Something like this:

git remote add monora https://github.com/monora/rgl.git # perhaps you have already this remote
git fetch monora

# last commit must be e70fdc
git log monora/78-bidirectional-graph

# Now in your local branch (bidirectional-1), which should have no changes:
git reset --hard monora/master                  # Bring branch to top of master
git reset --soft monora/78-bidirectional-graph  # Apply changes from my branch

Now your branch should have the desired changes e70fdc0 (check this).
Now commit these with my comment.

git add .
git commit -m'my comment...'
git push --force

git reset is often simpler than git rebase...
I hope, this works as expected.

[monora]

I forgot: If you do not want to loose the commits in your branch
bidirectional-1, you should clone and push it before doing git reset:

git checkout -b bidirectional-1-original bidirectional-1
git push
git checkout bidirectional-1

[jsjuni]

After the git reset --soft monora/78-bidirectional-graph I see this, which doesn't look right:

$ git status
On branch bidirectional-1
Your branch and 'origin/bidirectional-1' have diverged,
and have 3 and 13 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	modified:   lib/rgl/base.rb
	modified:   lib/rgl/bidirectional.rb
	deleted:    lib/rgl/bidirectional_adjacency.rb
	modified:   lib/rgl/path_builder.rb
	deleted:    test/bidirectional_graph_test.rb
	modified:   test/directed_graph_test.rb

Unless it matters to you, I don't mind if the changes aren't attributed to me. It was a fun little project but sic transit gloria mundi. If it's easier to just make the release yourself, feel free.

[monora]

sic transit gloria mundi
I found an easier way to share the glory:

git commit --amend --author "Steven Jenkins <j.s.jenkins@jpl.nasa.gov>"

See new commit 1d6c1b4

[monora]

Closed after changing commit author of #78 to @StevenJenkinsJPL

[monora]

Closed after changing commit author of #78 to @StevenJenkinsJPL.