Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support lstm and lstmCell operations #227

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

BruceDai
Copy link
Collaborator

@BruceDai BruceDai commented May 8, 2023

This pr is to

  1. add implementations for lstm and lstmCell operations
  2. firstly add three relevant test cases referring to ONNX conformance tests of lstm

@fdwr @huningxin @Honry PTAL, thanks.

5.51000023, 20.01000214,
19.11000061, 75.20999908,
],
[
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fdwr This third result containing every output from each time step

     [
        1, 8,
        1, 8,
        10.46900082,  58.02900696, 
        74.52900696, 518.94897461,
      ],

is different from the result "Y" of ONNX conformance tests in order, any suggestions? Thanks.

      "Y": {
        "type": "float32",
        "dims": [2, 1, 2, 2],
        "value": [[[[10.469000816345215, 58.02900695800781], [74.52900695800781, 518.948974609375]]], [[[1, 8], [1, 8]]]]
      },

Copy link

Choose a reason for hiding this comment

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

Does this pass now, after the changes, or is it still an issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's still an issue.

@@ -359,14 +359,16 @@ export class LstmCell extends Operation {
const zero = tf.scalar(0);
const starts = layout === MLLstmWeightLayout.iofg ?
{i: 0, o: hiddenSize, f: 2 * hiddenSize, g: 3 * hiddenSize} :
/*ifog*/ {i: 0, f: hiddenSize, o: 2 * hiddenSize, g: 3 * hiddenSize};
/*ifgo*/ {i: 0, f: hiddenSize, g: 2 * hiddenSize, 0: 3 * hiddenSize};
Copy link

Choose a reason for hiding this comment

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

What does 0: mean? I didn't know that key names were even allowed to start with digits :b (I figured they followed the same rules as other identifiers where they may contain digits but must start with a letter).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What does 0: mean?

It's a typo error, I've fixed it with last commit.

Copy link

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

I wondered if 0: should have been O: :b. Otherwise the rest looks fine, but mind you, I believe you understand LSTM better than I do after you finished reading that paper. So I'll have to somewhat trust that it's correct by vetting consistency with the other conformance tests.

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.

None yet

2 participants