-
Notifications
You must be signed in to change notification settings - Fork 206
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
Entailment pr #39
base: master
Are you sure you want to change the base?
Entailment pr #39
Conversation
model.add_node(RepeatVector(spad), name='Wh_n_cross_e', input='Wh_n') | ||
model.add_node(TimeDistributedDense(N,W_regularizer=l2(l2reg)), name='WY', input='Y') | ||
model.add_node(Activation('tanh'), name='M', inputs=['Wh_n_cross_e', 'WY'], merge_mode='sum') | ||
model.add_node(TimeDistributedDense(1,activation='softmax'), name='alpha', input='M') |
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.
softmax on 1 dim?
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.
Ok, it was a typo, replaced it with linear
Thanks a lot for the update! I'm a lot happier I think, a bit wary about the ugly looking code in rnn_input but I understand the neccessity and we'll be able to throw away that complexity again with the Keras1.0 port. So it's okay. :) But I'm still a bit leery about the ptscorer business. I think the basic sanity check is:
The other problem I see is that we use completely different ptscoring strategies when the to_n is involved - I don't understand that. I think we discussed just making the output dimension a parameter of mlp_ptscorer? Would that be inferior to the current solution? |
EDIT:
|
The entailment model implementation is not 100% final, I had to replace some hardcoded code by the SequenceSplit layer and the performance is not as good as before.