Skip to content

Conversation

@sidgoyal78
Copy link
Contributor

@sidgoyal78 sidgoyal78 commented May 17, 2018

I have found 2 issues:

  • In this chapter, we need to be able to read the embedding parameters. However, with the new high level API, we can't modify the train_loop to read the params. Hence, this current implementation doesn't work.

More precisely, if we look at the earlier implementation: https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/fluid/tests/book/test_label_semantic_roles.py#L200-L204

  • There is a problem with the learning rate layer inside optimizer. I get the error:
 File "/usr/local/lib/python2.7/dist-packages/paddle/fluid/optimizer.py", line 73, in _create_global_learning_rate
    "learning rate variable is create outside optimizer,"
TypeError: learning rate variable is create outside optimizer,can not create new learning rate variable for new program

For reference, see original implementation: https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/fluid/tests/book/test_label_semantic_roles.py#L173-L178

Potential fixes (which work)

  • Issue 1: We can make the embedding layer trainable.
  • Issue 2: We can get around the optimizer problem by having a straight-forward optimizer, which is done here.

@jetfuel
Copy link
Contributor

jetfuel commented May 21, 2018

@sidgoyal78 By the way, You will need to create a Makefile for this test. You can simply copy the Makefile from the other folder. Also we will need to add subdirectory in the parent Makefile like this
add_subdirectory(label_semantic_roles). Otherwise we will not run the test.

@sidgoyal78
Copy link
Contributor Author

Yes, @jetfuel. I will add it after resolving the aforementioned issues. As of now, we have it as a notest script.

@sidgoyal78 sidgoyal78 requested a review from jetfuel May 21, 2018 20:53
@sidgoyal78 sidgoyal78 merged commit 87ff95d into PaddlePaddle:develop May 24, 2018
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.

3 participants