fix gru, rnn, lstm test cases to match the specification and add some cases #920

fumihwh · 2018-05-09T15:18:52Z

lstm

cast zeros to np.float32. default np.float64 causes inconsistency of input's dtype.

lstm, rnn and gru

expand Y dim to match [seq_length, num_directions, batch_size, hidden_size]
add input seq_length = 2 's test case

# Conflicts: # onnx/backend/test/case/node/lstm.py

# Conflicts: # onnx/backend/test/case/node/gru.py # onnx/backend/test/case/node/lstm.py # onnx/backend/test/case/node/rnn.py # onnx/backend/test/data/node/test_gru_defaults/model.onnx # onnx/backend/test/data/node/test_gru_defaults/test_data_set_0/output_0.pb # onnx/backend/test/data/node/test_gru_with_initial_bias/model.onnx # onnx/backend/test/data/node/test_gru_with_initial_bias/test_data_set_0/output_0.pb # onnx/backend/test/data/node/test_lstm_defaults/model.onnx # onnx/backend/test/data/node/test_lstm_defaults/test_data_set_0/output_0.pb # onnx/backend/test/data/node/test_lstm_with_initial_bias/model.onnx # onnx/backend/test/data/node/test_lstm_with_initial_bias/test_data_set_0/output_0.pb # onnx/backend/test/data/node/test_lstm_with_peepholes/model.onnx # onnx/backend/test/data/node/test_lstm_with_peepholes/test_data_set_0/output_0.pb # onnx/backend/test/data/node/test_simple_rnn_defaults/model.onnx # onnx/backend/test/data/node/test_simple_rnn_defaults/test_data_set_0/output_0.pb # onnx/backend/test/data/node/test_simple_rnn_with_initial_bias/model.onnx # onnx/backend/test/data/node/test_simple_rnn_with_initial_bias/test_data_set_0/output_0.pb

AppVeyorBot · 2018-05-17T10:27:01Z

✅ Build onnx 0.3.3299 completed (commit f7ad67e7e7 by @fumihwh)

AppVeyorBot · 2018-05-17T10:54:55Z

✅ Build onnx 0.3.3301 completed (commit c0ee7c17b9 by @fumihwh)

# Conflicts: # onnx/backend/test/case/node/lstm.py # onnx/backend/test/data/node/test_lstm_defaults/model.onnx # onnx/backend/test/data/node/test_lstm_defaults/test_data_set_0/output_0.pb # onnx/backend/test/data/node/test_lstm_with_initial_bias/model.onnx # onnx/backend/test/data/node/test_lstm_with_initial_bias/test_data_set_0/output_0.pb

AppVeyorBot · 2018-05-19T15:34:36Z

❌ Build onnx 0.3.3393 failed (commit 88f45dc176 by @fumihwh)

linkerzhang · 2018-05-21T20:38:53Z

onnx/backend/test/case/node/rnn.py

-
-        H = self.f(np.dot(self.X, np.transpose(self.W)) + np.dot(self.H_0, self.R) + w_b + r_b)
-        return np.reshape(H, (self.num_directions, self.batch_size, self.hidden_size))
+        h_list = []


instead of having this step function/implementation, shall we have the output result explicitly set in the test case please? otherwise, I'm seeing this case is testing "test code" (this step function).

@linkerzhang
I didn't get you.
Basically, we need this numpy implemented function to calculate the output result.
Maybe the name of function step is not good for you?

haha, I was seeing this test case is testing the "step" function. I understood that it's used to generate the data. ignore this.

pk-g · 2018-05-21T21:01:50Z

onnx/backend/test/case/node/lstm.py


-            self.hidden_size = params[R].shape[-1]


Same as the other comment: Is there any specific reason why we are removing these parameters from the class and instead rely on local variables? this may put unnecessary limitations on reusing the values elsewhere.

pk-g · 2018-05-21T21:16:22Z

onnx/backend/test/case/node/gru.py


-            b = params[B] if B in params else np.zeros(2 * number_of_gates * self.hidden_size)


Is there any specific reason why we are removing these parameters from the class and instead rely on local variables? this may put unnecessary limitations on reusing the values elsewhere.

I think we don't need to reuse hidden_size. All things (init b, p, h, c) need hidden_size is done when initialize class.
Even until last version doesn't have it.
The author of this version wanted to use hidden_size in step (to reshape the result) and put it in to class.

Thanks, sounds good.

pk-g · 2018-05-21T23:43:26Z

onnx/backend/test/case/node/lstm.py

+        lstm = LSTM_Helper(X=x, W=w, R=r, B=b)
+        output = lstm.step()
+
+        output_precomputed = np.array([[[[-0.09183919, -0.20181324, 0.04475278, -0.12022446,


Where are these numbers coming from and what is the need to have them here instead of relying on reference implementation? Using precomputed numbers may create unnecessary complications going forward. For instance, If a particular framework generates numbers that do not match these numbers in future, how can we resolve conflicts?

It comes from pytorch.
Yes, make sense. I will remove this.

# Conflicts: # onnx/backend/test/case/node/gru.py # onnx/backend/test/case/node/lstm.py # onnx/backend/test/case/node/rnn.py

AppVeyorBot · 2018-05-22T02:53:57Z

❌ Build onnx 0.3.3468 failed (commit a88545a49f by @fumihwh)

# Conflicts: # onnx/backend/test/case/node/gru.py # onnx/backend/test/case/node/lstm.py # onnx/backend/test/data/node/test_gru_defaults/model.onnx # onnx/backend/test/data/node/test_gru_with_initial_bias/model.onnx # onnx/backend/test/data/node/test_lstm_defaults/model.onnx # onnx/backend/test/data/node/test_lstm_with_initial_bias/model.onnx # onnx/backend/test/data/node/test_lstm_with_peepholes/model.onnx # onnx/backend/test/data/node/test_simple_rnn_defaults/model.onnx # onnx/backend/test/data/node/test_simple_rnn_with_initial_bias/model.onnx

AppVeyorBot · 2018-05-23T14:57:30Z

❌ Build onnx 0.3.3536 failed (commit ec730aaec7 by @fumihwh)

AppVeyorBot · 2018-05-23T16:24:46Z

❌ Build onnx 0.3.3538 failed (commit 8953279c84 by @fumihwh)

pk-g

Looks good to me in general.

…h the specification and add some cases (onnx/onnx#920) onnx/onnx@d2a46da

… cases (onnx#920) * fix lstm test cases to match the specification and add precomputed case * add rnn and update test cases * add and update gru test cases * update doc * remove lstm precomputed test case * update test cases * update doc and bug fix * fix mypy

fumihwh added 2 commits May 10, 2018 00:11

fix lstm test cases to match the specification and add precomputed case

2edc887

Merge branch 'master' into fix-lstm-test-cases

a27104d

This was referenced May 9, 2018

[auto] onnx-pr-920 onnxbot/onnx-fb-universe#2001

Open

[auto] onnx-pr-920 onnx/onnx-coreml#268

Closed

fumihwh added 2 commits May 12, 2018 23:24

Merge branch 'master' into fix-lstm-test-cases

571a85d

add rnn and update test cases

9fbaacb

fumihwh changed the title ~~fix lstm test cases to match the specification and add precomputed case~~ fix rnn, lstm test cases to match the specification and add some cases May 12, 2018

add and update gru test cases

02ef8dd

fumihwh changed the title ~~fix rnn, lstm test cases to match the specification and add some cases~~ fix gru, rnn, lstm test cases to match the specification and add some cases May 13, 2018

fumihwh added 3 commits May 13, 2018 22:55

Merge branch 'master' into fix-lstm-test-cases

d439fc2

Merge branch 'master' into fix-lstm-test-cases

8500a97

Merge branch 'master' into fix-lstm-test-cases

8e90dc2

# Conflicts: # onnx/backend/test/case/node/lstm.py

prasanthpul requested a review from ebarsoum May 15, 2018 20:44

update doc

018ca28

linkerzhang requested a review from pk-g May 19, 2018 17:52

bddppq requested a review from houseroad May 21, 2018 19:07

linkerzhang reviewed May 21, 2018

View reviewed changes

pk-g suggested changes May 21, 2018

View reviewed changes

fumihwh added 2 commits May 22, 2018 11:40

remove lstm precomputed test case

ef5623d

Merge branch 'master' into fix-lstm-test-cases

b19ebdb

# Conflicts: # onnx/backend/test/case/node/gru.py # onnx/backend/test/case/node/lstm.py # onnx/backend/test/case/node/rnn.py

update test cases

ad37bc2

update doc and bug fix

3914434

pk-g approved these changes May 23, 2018

View reviewed changes

linkerzhang approved these changes May 23, 2018

View reviewed changes

fumihwh and others added 2 commits May 24, 2018 07:25

fix mypy

fb3cf2a

Merge branch 'master' into fix-lstm-test-cases

1f58c68

linkerzhang merged commit d2a46da into onnx:master May 24, 2018

onnxbot added a commit to onnx/onnx-coreml that referenced this pull request May 24, 2018

[auto] Update onnx to d2a46da - fix gru, rnn, lstm test cases to matc…

b6b84e8

…h the specification and add some cases (onnx/onnx#920) onnx/onnx@d2a46da

onnxbot added a commit to pytorch/pytorch that referenced this pull request May 24, 2018

[auto] Update onnx to d2a46da - fix gru, rnn, lstm test cases to matc…

43d87af

…h the specification and add some cases (onnx/onnx#920) onnx/onnx@d2a46da

fumihwh deleted the fix-lstm-test-cases branch May 24, 2018 07:21

weiyangfb pushed a commit to weiyangfb/pytorch that referenced this pull request Jun 11, 2018

[auto] Update onnx to d2a46da - fix gru, rnn, lstm test cases to matc…

19ece46

…h the specification and add some cases (onnx/onnx#920) onnx/onnx@d2a46da

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix gru, rnn, lstm test cases to match the specification and add some cases #920

fix gru, rnn, lstm test cases to match the specification and add some cases #920

fumihwh commented May 9, 2018 •

edited

Loading

AppVeyorBot commented May 17, 2018

AppVeyorBot commented May 17, 2018

AppVeyorBot commented May 19, 2018

linkerzhang May 21, 2018

fumihwh May 22, 2018

linkerzhang May 23, 2018

pk-g May 21, 2018 •

edited

Loading

pk-g May 21, 2018

fumihwh May 22, 2018

pk-g May 23, 2018

pk-g May 21, 2018 •

edited

Loading

fumihwh May 22, 2018

AppVeyorBot commented May 22, 2018

AppVeyorBot commented May 23, 2018

AppVeyorBot commented May 23, 2018

pk-g left a comment


		b = params[B] if B in params else np.zeros(2 * number_of_gates * self.hidden_size)

fix gru, rnn, lstm test cases to match the specification and add some cases #920

fix gru, rnn, lstm test cases to match the specification and add some cases #920

Conversation

fumihwh commented May 9, 2018 • edited Loading

AppVeyorBot commented May 17, 2018

AppVeyorBot commented May 17, 2018

AppVeyorBot commented May 19, 2018

linkerzhang May 21, 2018

Choose a reason for hiding this comment

fumihwh May 22, 2018

Choose a reason for hiding this comment

linkerzhang May 23, 2018

Choose a reason for hiding this comment

pk-g May 21, 2018 • edited Loading

Choose a reason for hiding this comment

pk-g May 21, 2018

Choose a reason for hiding this comment

fumihwh May 22, 2018

Choose a reason for hiding this comment

pk-g May 23, 2018

Choose a reason for hiding this comment

pk-g May 21, 2018 • edited Loading

Choose a reason for hiding this comment

fumihwh May 22, 2018

Choose a reason for hiding this comment

AppVeyorBot commented May 22, 2018

AppVeyorBot commented May 23, 2018

AppVeyorBot commented May 23, 2018

pk-g left a comment

Choose a reason for hiding this comment

fumihwh commented May 9, 2018 •

edited

Loading

pk-g May 21, 2018 •

edited

Loading

pk-g May 21, 2018 •

edited

Loading