-
Notifications
You must be signed in to change notification settings - Fork 267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Massive improvement. LGTM. Mostly comments about comments.
python/myelin/flow.py
Outdated
def add_attr(self, name, value): | ||
if type(value) is bool: | ||
if value == True: value = 1 | ||
elif value == False: value = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorter but maybe not more readable:
value = int(value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is simpler. Done.
sling/myelin/rnn.cc
Outdated
tf.RandomOrtho(x2o); | ||
tf.RandomOrtho(h2o); | ||
|
||
// i = sigmoid(x * x2i + h_in * h2i + c_in * c2i + bi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might just be a copy paste bug in the comment inherited from the DRAGNN LSTM.
c_in
and c2i
do not appear in the expression below. Moreover, c2i has not been defined/assigned yet, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. That is a copy/paste bug. The standard LSTM does not have peephole connections.
sling/nlp/parser/parser-trainer.cc
Outdated
@@ -315,20 +338,13 @@ void ParserTrainer::Parse(Document *document) const { | |||
d = action.delegate; | |||
} | |||
|
|||
// Shift or stop if predicted action is invalid. | |||
// Shift if predicted action is not invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalid, not invalid or not valid? Life is just more exciting with double negations... :-)
sling/nlp/parser/parser.cc
Outdated
d = action.delegate; | ||
} | ||
|
||
// Shift if predicted action is not invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not invalid or not valid or invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is nothing not wrong with this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review and the comments.
python/myelin/flow.py
Outdated
def add_attr(self, name, value): | ||
if type(value) is bool: | ||
if value == True: value = 1 | ||
elif value == False: value = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is simpler. Done.
sling/myelin/rnn.cc
Outdated
tf.RandomOrtho(x2o); | ||
tf.RandomOrtho(h2o); | ||
|
||
// i = sigmoid(x * x2i + h_in * h2i + c_in * c2i + bi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. That is a copy/paste bug. The standard LSTM does not have peephole connections.
sling/nlp/parser/parser.cc
Outdated
d = action.delegate; | ||
} | ||
|
||
// Shift if predicted action is not invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is nothing not wrong with this :)
sling/nlp/parser/parser.cc
Outdated
d = action.delegate; | ||
} | ||
|
||
// Shift if predicted action is not invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
This PR changes the parser flow file format, so I have trained and deployed new pre-trained models: 1-layer 128-dim bi-directional LSTM (caspar-accurate.flow):
3-layer 192-dim bi-directional LSTM (caspar-accurate.flow):
|
I have implemented a new RNN module for the parser that implements a number of different RNN architectures:
The RNN module also supports:
The best parser model with one bi-directional LSTM layer produces the following results:
You can make a more accurate model with a 3-layer 192-dim bi-directional LSTM:
For comparison, the baseline for the old PyTorch-based trainer is:
Details: