Refactoring Graph Warp Module #340

mottodora · 2019-03-13T10:07:40Z

Resolve #329

Add unit test of preprocesor
Refactor preprocessor
Add unit test of GWM model
Refactor GWM model

mottodora · 2019-03-19T02:08:40Z

I finish implementation. Please review.

codecov-io · 2019-03-23T04:51:35Z

Codecov Report

Merging #340 into master will increase coverage by 1.29%.
The diff coverage is 94.96%.

@@            Coverage Diff             @@
##           master     #340      +/-   ##
==========================================
+ Coverage   81.65%   82.95%   +1.29%     
==========================================
  Files         210      211       +1     
  Lines        9647     9813     +166     
==========================================
+ Hits         7877     8140     +263     
+ Misses       1770     1673      -97

corochann · 2019-04-05T06:38:46Z

chainer_chemistry/dataset/preprocessors/common.py

+def mol_basic_info_feature(mol, atom_array, adj):
+    n_atoms = mol.GetNumAtoms()
+    assert n_atoms == len(atom_array)
+    n_edges = adj.sum()


Just comment: actually this is actual number of edges * 2.

done in new PR

corochann · 2019-04-05T06:47:34Z

chainer_chemistry/dataset/preprocessors/common.py

+
+
+def construct_supernode_feature(mol, atom_array, adj, feature_functions=None):
+                                # largest_atomic_number=MAX_ATOMIC_NUM, out_size=-1):


also delete docstring

done in new PR

corochann · 2019-04-05T06:53:59Z

chainer_chemistry/models/gwm.py

@@ -22,80 +156,65 @@ class GWM(chainer.Chain):
            number of super-node observation attributes
        n_edge_types (int): number of edge types witin graphs.
        dropout_ratio (default=0.5); if > 0.0, perform dropout
-        tying_flag (default=false): enable if you want to share params across layers
+        tying_flag (default=false): enable if you want to share params across
+            layers


add docstring for activation, wgu_activation and gtu_activation

done in new PR

corochann · 2019-04-05T06:55:27Z

chainer_chemistry/models/gwm.py

        if tying_flag:
-            num_layer = 1
+            n_layers = 1


It makes bug. self.n_layers must be set before override this value.

Instead, refactor to remove n_layers dependency and do not use step in call method.
TODO later.

corochann · 2019-04-05T06:56:50Z

chainer_chemistry/models/gwm.py

-        :param adj: minibatch by bond_types by num_nodes by num_nodes 1/0 array.
-                Adjacency matrices over several bond types
+        :param adj: minibatch by bond_types by num_nodes by num_nodes 1/0
+                array. Adjacency matrices over several bond types


can you change to Google docstring format?

done in new PR

corochann · 2019-04-05T06:58:50Z

tests/models_tests/test_gwm.py

+
+
+def check_forward(gwm, embed_atom_data, new_embed_atom_data, supernode):
+    gwm.GRU_local.reset_state()


gwm.reset_state() is better

done in new PR

corochann · 2019-04-05T06:59:41Z

tests/models_tests/test_gwm.py

+
+def check_backward(gwm, embed_atom_data, new_embed_atom_data, supernode,
+                   y_grad, supernode_grad):
+    gwm.GRU_local.reset_state()


gwm.reset_state() is better

done in new PR

corochann · 2019-04-05T07:07:00Z

chainer_chemistry/models/gwm.py

+        return merged
+
+
+class SuperNodeTransmitterUnit(chainer.Chain):


can you add docstring?

done in new PR

corochann · 2019-04-05T07:07:04Z

chainer_chemistry/models/gwm.py

+        return g_trans
+
+
+class GraphTransmitterUnit(chainer.Chain):


can you add docstring?

done in new PR

corochann · 2019-04-05T07:07:09Z

chainer_chemistry/models/gwm.py

@@ -5,11 +5,145 @@
 from chainer_chemistry.links import GraphLinear


+class WarpGateUnit(chainer.Chain):


can you add docstring?

done in new PR

corochann · 2019-04-05T07:08:37Z

chainer_chemistry/models/gwm.py

+        elif output_type == 'super':
+            LinearFunc = links.Linear
+        else:
+            raise ValueError


show proper error message.
ValueError('output_type = {} is unexpected. graph or super is supported.'.format(output_type))

done in new PR

corochann · 2019-04-05T07:25:10Z

chainer_chemistry/models/gwm.py

+        super(SuperNodeTransmitterUnit, self).__init__()
+        with self.init_scope():
+            self.F_super = links.Linear(in_size=hidden_dim_super,
+                                        out_size=hidden_dim)


Originally it was out_size=hidden_dim_super, is it ok?

Original code uses F_super for 2 place: update super node feature itself & message to node for transmission.

F_super is separated and I think it's okay.

corochann · 2019-04-05T07:30:46Z

chainer_chemistry/models/gwm.py

+        # for local updates
+        g_trans = self.F_super(g)
+        # intermediate_h_super.shape == (mb, self.hidden_dim)
+        g_trans = functions.tanh(g_trans)


How about return g_trans here, and let "expand_dims and broadcast" later when necessary in other module?
So that we can remove n_atoms argument dependency.

corochann · 2019-04-05T07:33:18Z

chainer_chemistry/models/gwm.py

+                *[links.Linear(in_size=hidden_dim_super,
+                               out_size=hidden_dim_super)
+                  for _ in range(n_layers)]
+            )


do you separate F_super ?

It is different from original code, but separating F_super is better and it's ok.

corochann · 2019-04-05T07:39:03Z

chainer_chemistry/models/gwm.py

+        # update for attention-message B h_i
+        # h1.shape == (mb, atom, n_heads * ch)
+        # Bh_i.shape == (mb, atom, self.n_heads * self.hidden_dim_super)
+        Bh_i = self.B(h1)


I think broadcast h1 is redundant and can be skipped to reduce computation

done in new PR

corochann · 2019-04-05T07:39:50Z

chainer_chemistry/models/gwm.py

+        with self.init_scope():
+            self.V_super = links.Linear(hidden_dim * n_heads, hidden_dim * n_heads)
+            self.W_super = links.Linear(hidden_dim * n_heads, hidden_dim_super)
+            self.B = GraphLinear(n_heads * hidden_dim, n_heads * hidden_dim_super)


self.B = GraphLinear(hidden_dim, n_heads * hidden_dim_super) when we skip h1 broadcast

done in new PR

corochann · 2019-04-05T07:44:32Z

chainer_chemistry/models/gwm.py

+        # intermediate_h.shape == (mb, self.n_heads * ch)
+        h_trans = self.V_super(attention_sum)
+        # compress heads
+        h_trans = self.W_super(h_trans)


what is the meaning of applying linear operation twice?

I think V_super is not necessary

done in new PR

corochann · 2019-04-05T07:57:53Z

chainer_chemistry/models/gwm.py

+        self.activation = activation
+
+    def __call__(self, h, g):
+        z = self.H(h) + self.G(g)


I think we can calculate self.G(g) as Linear layer followed by broadcast to each atom.

corochann · 2019-05-17T09:50:16Z

Can you update code? @mottodora

corochann · 2019-07-19T02:18:56Z

I will take over.

corochann · 2019-07-25T01:22:33Z

chainer_chemistry/models/gwm.py

+        super(SuperNodeTransmitterUnit, self).__init__()
+        with self.init_scope():
+            self.F_super = links.Linear(in_size=hidden_dim_super,
+                                        out_size=hidden_dim)


Original code uses F_super for 2 place: update super node feature itself & message to node for transmission.

corochann · 2019-07-26T02:11:18Z

tests/models_tests/test_gwm.py

+
+def check_backward(gwm, embed_atom_data, new_embed_atom_data, supernode,
+                   y_grad, supernode_grad):
+    gwm.GRU_local.reset_state()


done in new PR

corochann · 2019-07-26T02:11:33Z

tests/models_tests/test_gwm.py

+
+
+def check_forward(gwm, embed_atom_data, new_embed_atom_data, supernode):
+    gwm.GRU_local.reset_state()


done in new PR

corochann · 2019-07-26T02:11:46Z

chainer_chemistry/models/gwm.py

-        :param adj: minibatch by bond_types by num_nodes by num_nodes 1/0 array.
-                Adjacency matrices over several bond types
+        :param adj: minibatch by bond_types by num_nodes by num_nodes 1/0
+                array. Adjacency matrices over several bond types


done in new PR

corochann · 2019-07-26T02:12:28Z

chainer_chemistry/models/gwm.py

+                *[links.Linear(in_size=hidden_dim_super,
+                               out_size=hidden_dim_super)
+                  for _ in range(n_layers)]
+            )


It is different from original code, but separating F_super is better and it's ok.

corochann · 2019-07-26T02:14:23Z

chainer_chemistry/models/gwm.py

+        # update for attention-message B h_i
+        # h1.shape == (mb, atom, n_heads * ch)
+        # Bh_i.shape == (mb, atom, self.n_heads * self.hidden_dim_super)
+        Bh_i = self.B(h1)


done in new PR

corochann · 2019-07-26T02:14:30Z

chainer_chemistry/models/gwm.py

+        # intermediate_h.shape == (mb, self.n_heads * ch)
+        h_trans = self.V_super(attention_sum)
+        # compress heads
+        h_trans = self.W_super(h_trans)


done in new PR

corochann · 2019-07-26T02:14:55Z

chainer_chemistry/models/gwm.py

+
+        h_j = functions.expand_dims(h, 1)
+        # h_j.shape == (mb, self.n_heads, atom, ch)
+        h_j = functions.broadcast_to(h_j, (mb, self.n_heads, atom, ch))


apply V_super instead of broadcast

corochann · 2019-07-26T02:15:40Z

chainer_chemistry/models/gwm.py

@@ -22,80 +156,65 @@ class GWM(chainer.Chain):
            number of super-node observation attributes
        n_edge_types (int): number of edge types witin graphs.
        dropout_ratio (default=0.5); if > 0.0, perform dropout
-        tying_flag (default=false): enable if you want to share params across layers
+        tying_flag (default=false): enable if you want to share params across
+            layers


done in new PR

corochann · 2019-07-26T02:18:31Z

chainer_chemistry/models/gwm.py

        if tying_flag:
-            num_layer = 1
+            n_layers = 1


Instead, refactor to remove n_layers dependency and do not use step in call method.
TODO later.

mottodora added 2 commits March 13, 2019 17:13

add unit tests of construct_super_node_feature

4a681cf

refactor construct_supernode_feature

ae15560

mottodora force-pushed the refactor-gwm branch from a3e57bc to ae15560 Compare March 14, 2019 04:05

mottodora added 4 commits March 14, 2019 14:23

flake8

817fba4

add unit test of GWM

620c21c

🐛 fix

30e6737

Add transmitter and WarpGate unit

80dad51

mottodora force-pushed the refactor-gwm branch from b2bed4b to 80dad51 Compare March 15, 2019 14:29

mottodora added 5 commits March 16, 2019 00:08

Add graph invariant test

803fdfe

fix for backward test

ec53738

delete n_layer arguments

8d3e150

add GPU test

889ddb4

change rtol

591b442

mottodora marked this pull request as ready for review March 19, 2019 02:07

natsukium self-requested a review March 22, 2019 10:12

fix for old chainer

80986ff

mottodora force-pushed the refactor-gwm branch from a8923fc to 80986ff Compare March 23, 2019 04:36

corochann self-requested a review April 5, 2019 06:35

corochann requested changes Apr 5, 2019

View reviewed changes

corochann reviewed Apr 5, 2019

View reviewed changes

corochann approved these changes Jul 19, 2019

View reviewed changes

corochann merged commit 85aae47 into chainer:master Jul 19, 2019

corochann mentioned this pull request Jul 19, 2019

Refactoring Graph Warp Module #367

Closed

corochann reviewed Jul 26, 2019

View reviewed changes

mottodora added this to the 0.6.0 milestone Sep 11, 2019



		def construct_supernode_feature(mol, atom_array, adj, feature_functions=None):
		# largest_atomic_number=MAX_ATOMIC_NUM, out_size=-1):



		def check_forward(gwm, embed_atom_data, new_embed_atom_data, supernode):
		gwm.GRU_local.reset_state()

		@@ -5,11 +5,145 @@
		from chainer_chemistry.links import GraphLinear


		class WarpGateUnit(chainer.Chain):

Refactoring Graph Warp Module #340

Refactoring Graph Warp Module #340

Conversation

mottodora commented Mar 13, 2019 • edited Loading

mottodora commented Mar 19, 2019

codecov-io commented Mar 23, 2019

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corochann commented May 17, 2019

corochann commented Jul 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mottodora commented Mar 13, 2019 •

edited

Loading