Graph class #13

npochhi · 2018-05-21T05:29:59Z

Description

Implements all the basic classes

Relevant Issues : (required)
Relevant PRs : (optional)
Type of change :
- New feature
- Bug fix for existing feature
- Code quality improvement
- Addition or Improvement of tests
- Addition or Improvement of documentation

athityakumar · 2018-05-25T12:41:53Z

lib/networkx/digraph.rb

+    def number_of_edges
+      num = 0
+      @adj.each { |_, v| num += v.length }
+      num


Not sure if how this is better performance-wise, but you can probably also try it out.

def number_of_edges @adj.values.map(&:length).inject(:+) end

athityakumar · 2018-05-25T12:48:34Z

lib/networkx/digraph.rb

+      @adj[node].length
+    end
+
+    # Returns the reversed version of a the graph


a the -> the

In general, just ~~double-check~~ triple-check the YARD comments for any typos. 😄

athityakumar · 2018-05-25T12:53:08Z

lib/networkx/digraph.rb

+    #   graph.to_undirected
+    def to_undirected
+      new_graph = NetworkX::Graph.new(@graph)
+      @nodes.each_key { |k| new_graph.add_node(k, @nodes[k]) }


Any specific reason for not using the more obvious each rather than each_key when we anyway have to access the value (@nodes[k]) too? Maybe change it to:

@nodes.each { |k, v| new_graph.add_node(k, v) }

athityakumar · 2018-05-25T12:53:59Z

lib/networkx/digraph.rb

+      @nodes.each_key { |k| new_graph.add_node(k, @nodes[k]) }
+      @adj.each_key do |k|
+        @adj[k].each_key { |v| new_graph.add_edge(v, k, @adj[k][v]) }
+      end


Again, use Hash#each rather than Hash#each_key 😄

athityakumar · 2018-05-25T12:55:32Z

lib/networkx/digraph.rb

+        sub_graph = NetworkX::DiGraph.new(@graph)
+        nodes.each do |u, _|
+          raise KeyError, "#{u} does not exist in the current graph!" unless @nodes.key?(u)
+          sub_graph.add_node(u, @nodes[u])


Yet again, each vs each_key 😄

athityakumar · 2018-05-25T12:56:23Z

lib/networkx/digraph.rb

+      end
+    end
+
+    # Returns subgraph conisting of given edges


Typo: conisting -> consisting

athityakumar · 2018-05-25T13:01:17Z

lib/networkx/graph.rb

+    #
+    # @param edges [Array<Object, Object>] the array of edges
+    # @param weight [Array<Integer>] the array of weights
+    def add_weighted_edges(edges, weights)


The args provided in the documentation and the args of the method don't match. Have a look at this again.

athityakumar · 2018-05-25T13:04:22Z

lib/networkx/graph.rb

+    #
+    # @param node [Object] the node to be checked
+    def node?(node)
+      @node.key?(node)


Typo: @node -> @nodes

How did this not come up in the unit tests? 🤔

This method was quite trivial like some other methods like edge? etc. Do I need to add tests for such trivial methods as well? 😥

athityakumar · 2018-05-25T13:05:32Z

lib/networkx/graph.rb

+    # @param node_1 [Object] the first node of the edge to be checked
+    # @param node_2 [Object] the second node of the edge to be checked
+    def edge?(node_1, node_2)
+      return true if @nodes.key?(node_1) && @adj[node_1].key?(node_2)


Why not re-use the key? method here instead of writing @nodes.key? again?

athityakumar · 2018-05-25T13:07:20Z

lib/networkx/graph.rb

+    # @param node_2 [Object] the second node of the edge to be checked
+    def edge?(node_1, node_2)
+      return true if @nodes.key?(node_1) && @adj[node_1].key?(node_2)
+      false


Are the explicit true and false really required here? The method could simply be just 1-line. 😄

def edge?(node_1, node_2) node?(node_1) && @adj[node_1].key?(node_2) end

athityakumar · 2018-05-25T13:09:45Z

lib/networkx/graph.rb

+    #
+    # @param node [Object] the node whose data is to be fetched
+    def get_node_data(node)
+      return @nodes[node] if @nodes.key?(node)


Again, the above node? method could be used rather than re-writing @nodes.key?(node)

athityakumar · 2018-05-25T13:09:56Z

lib/networkx/graph.rb

+    # @param node [Object] the node whose data is to be fetched
+    def get_node_data(node)
+      return @nodes[node] if @nodes.key?(node)
+      raise ArgumentError, 'No such node exists!'


Maybe the other way round would be more readable?

def get_node_data(node) raise ArgumentError, 'No such node exists' unless node?(node) @nodes[node] end

Follow this for the next 2 methods too (with KeyError, and node? instead of @nodes.key). Let's keep the review DRY. 😉

athityakumar · 2018-05-25T13:12:49Z

lib/networkx/graph.rb

+      num = 0
+      @adj.each { |_, v| num += v.length }
+      num / 2
+    end


Refer to the map + inject combination I had mentioned in an earlier comment.

athityakumar · 2018-05-25T13:14:53Z

lib/networkx/graph.rb

+    # @example
+    #   graph.size(true)
+    #
+    # @praram is_weighted [Bool] if we want weighted size of unweighted size


Make same documentation comment changes, as mentioned to the same method in the previous file. (Logic explanation moved to params + typo)

athityakumar · 2018-05-25T13:18:50Z

lib/networkx/multidigraph.rb

+    def new_edge_key(node_1, node_2)
+      return 0 if @adj[node_1][node_2].nil?
+      key = @adj[node_1][node_2].length
+      key += 1 while @adj[node_1][node_2].key?(key)


Is this to handle deletion on intermediate key edges? If so, you can use maintain a last_key and use it here rather than #length and loop for coming to last_key.

Then we would have to store the last_key attribute for every edge. Wouldn't that be an overhead? Also, we'd like to reuse the keys after deleting them.

Also, I took the reference of networkx for the implementation of this method.

athityakumar · 2018-05-25T13:24:04Z

lib/networkx/multidigraph.rb

+      raise KeyError, "#{node_2} is not a valid node" unless @nodes.key?(node_2)
+      raise KeyError, 'The given edge is not a valid one.' unless @adj[node_1].key?(node_2)
+      raise KeyError, 'The given edge is not a valid one.' unless @adj[node_1][node_2].key?(key)
+      @adj[node_1][node_2].delete(key)


These raise checks come up in the DiGraph class too, right? Maybe the raise checks can be put as a separate method to be called from both remove_edge methods.

athityakumar · 2018-05-25T13:25:31Z

lib/networkx/multidigraph.rb

+      super(node_1, node_2) if key.nil?
+      return true if @nodes.key?(node_1) && @adj[node_1].key?(node_2) && @adj[node_1][node_2].key?(key)
+      false
+    end


Refer to the "explicit true-false" comment above.

athityakumar · 2018-05-25T13:27:53Z

lib/networkx/multidigraph.rb

+    #   graph.to_undirected
+    def to_undirected
+      graph = NetworkX::Graph.new(@graph)
+      @nodes.each { |node, node_attr| graph.add_node(node, node_attr) }


Isn't MultiDiGraph#to_undirected supposed to written Multigraph`? Have a good look at this.

Hmm, had a look at the to_multigraph method. But in networkx, the #to_undirected is intended to return MultiGraph.

Reference: https://networkx.github.io/documentation/stable/reference/classes/generated/networkx.MultiDiGraph.to_undirected.html

@MridulS @v0dro - Any comments on which one might be more intuitive?

athityakumar · 2018-05-25T13:28:56Z

lib/networkx/multidigraph.rb

+          @adj[node_1][node_2].each_key { |key| edge_attrs.merge!(@adj[node_1][node_2][key]) }
+          graph.add_edge(node_1, node_2, edge_attrs)
+        end
+      end


Remember the each vs each_key comment?

athityakumar · 2018-05-25T13:32:42Z

lib/networkx/multidigraph.rb

+      graph
+    end
+
+    # Returns the reversed version of a the graph


Typo: a the -> the

athityakumar · 2018-05-25T13:33:29Z

lib/networkx/multidigraph.rb

+      @adj.each_key do |u|
+        @adj[u].each_key { |v| @adj[u][v].each_key { |k| new_graph.add_edge(v, u, @adj[u][v][k]) } }
+      end
+      new_graph


... And the each vs each_key again

athityakumar · 2018-05-25T13:34:14Z

lib/networkx/multidigraph.rb

+      in_degree_no = 0
+      @pred[node].each_key { |u| in_degree_no += @pred[node][u].length }
+      in_degree_no
+    end


Refer to the map + inject comment

athityakumar · 2018-05-25T13:34:45Z

lib/networkx/multidigraph.rb

+    def out_degree(node)
+      out_degree_no = 0
+      @adj[node].each_key { |u| out_degree_no += @adj[node][u].length }
+      out_degree_no


Refer to the map + inject comment above

athityakumar · 2018-05-25T13:35:34Z

lib/networkx/multidigraph.rb

+      no_of_edges = 0
+      @adj.each_key { |node_1| @adj[node_1].each_key { |node_2| no_of_edges += @adj[node_1][node_2].length } }
+      no_of_edges
+    end


As you might have guessed by now, map + inject again. 😉

athityakumar · 2018-05-25T13:38:01Z

lib/networkx/multigraph.rb

+      end
+    end
+  end
+end


Refer to the comments mentioned in the MultiDiGraph class

athityakumar

Hey @npochhi. Pretty good PR, but there have been lots of suggestions as there are multiple Graph classes with very similar things to fix. To mention some,

map + inject
each vs each_key
Explicit true/false returns from a method
Typos

Also, I found a method that had a typo in it's code (variable). That's what unit tests are supposed to prevent, but Travis CI build seemingly passed. Which means, the unit-tests are not complete. Can you please add the unit-tests to all the methods?

athityakumar

Good work with this PR. 👍

npochhi added 15 commits March 2, 2018 04:08

Added remove_node/s methods and modified the original data structure

810c30e

Resolved Travis CI offenses

ef758f7

Added rspec tests for Graph class

f13c45d

Added YARD Doc, removed offenses and improved rspec tests

687c57f

Added examples

386a834

Changed docs a little bit

53e1175

Renamed graph-spec

7cc8dc9

Reverted renaming changes

5ff9f36

Completed graph.rb

b64f662

Added remaining tests for graph class

17a3daa

Added digraph class

4aa911c

Added multigraph class

c45ef20

Added multidigraph class

fe3aabf

Added multigraph, multidigraph specs

3f5f9e1

Added comments

851f4eb

npochhi changed the base branch from master to graph-class-formation May 21, 2018 05:30

Removed offenses

2d5762c

npochhi force-pushed the graph-class branch 4 times, most recently from ee8c2c8 to 2d5762c Compare May 21, 2018 17:54

Merge branch 'graph-class-formation' into graph-class

0c340c5

npochhi closed this May 21, 2018

npochhi reopened this May 21, 2018

npochhi closed this May 21, 2018

npochhi reopened this May 21, 2018

npochhi closed this May 22, 2018

npochhi reopened this May 22, 2018

removed offenses

fb48b08

athityakumar reviewed May 25, 2018

View reviewed changes

lib/networkx/digraph.rb Outdated

end

end

# Returns subgraph conisting of given edges

Copy link

Member

athityakumar May 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: conisting -> consisting

athityakumar reviewed May 25, 2018

View reviewed changes

lib/networkx/multidigraph.rb Outdated

graph

end

# Returns the reversed version of a the graph

Copy link

Member

athityakumar May 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: a the -> the

athityakumar reviewed May 25, 2018

View reviewed changes

lib/networkx/multigraph.rb

end

end

end

end

Copy link

Member

athityakumar May 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer to the comments mentioned in the MultiDiGraph class

athityakumar requested changes May 25, 2018

View reviewed changes

npochhi added 3 commits June 4, 2018 23:57

Changed the code acc to reviews

3531186

Removed all offenses

cc18b3a

Added leftover comments

cb7238c

athityakumar approved these changes Jun 9, 2018

View reviewed changes

athityakumar merged commit 2dfe5be into SciRuby:graph-class-formation Jun 9, 2018

Graph class #13

Graph class #13

Uh oh!

Conversation

npochhi commented May 21, 2018

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athityakumar May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athityakumar left a comment

Choose a reason for hiding this comment

Uh oh!

athityakumar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

athityakumar May 25, 2018 •

edited

Loading