CRF for keras 2.x #76
Conversation
|
@farizrahman4u The tensorflow and keras version are not compatible in .travis checking. I think tensorflow should be update to version 1.x in .travis. |
|
@linxihui Thanks for pointing it out.. please submit a PR to fix travis config so that you get the Github credits. |
|
Cool stuff! Can anyone review? |
keras_contrib/layers/crf.py
Outdated
| return acc | ||
|
|
||
| @staticmethod | ||
| def log_sum_exp(x, axis=-1): |
There was a problem hiding this comment.
Since keras-team/keras#6346 has been merged, could it be integrated in this PR?
There was a problem hiding this comment.
Sure, will change it soon.
There was a problem hiding this comment.
yes, it is merged fairly recently, but people who not using the master branch (e.g., install using pip) will have trouble. So I keep it. Maybe remove it sometime later.
There was a problem hiding this comment.
Ok let's keep this in mind and open an issue linked to this PR to make sure we make the change when this will be merged?
There was a problem hiding this comment.
Then I probably should just change it. Actually, there is a bug when using K.logsumexp with tensorflowbackend, when passed a placeholder, it losses the shape information, i.e.,K.int_shapegot None. This is due to the fact thatK.logsumexpuses an deprecidated argument. I'll make a PR tokeras` and will make the change here one it is merged.
See keras-team/keras#6516
examples/conll2000_chunking_crf.py
Outdated
|
|
||
| import numpy | ||
| from collections import Counter | ||
| from sklearn.metrics import classification_report |
There was a problem hiding this comment.
Should we try to avoid those dependencies (sklearn and nltk)?
I agree this is very convenient and not hard to install but it could be easy to add a new dataset as in #25.
There was a problem hiding this comment.
sklearn is not necessary, as I just use the classification_report to print the result in a nice way. nltk is here for getting the data. My original idea was a bit on the opposite, avoiding having the same dataset in multiple packages or writing duplicate script to load data, but instead using an existing package, which also saves the effort of licensing. But I understand the disadvantage as well. I'll change it, as it probably fits better to the Keras way.
|
@tboquet using |
|
@tboquet @farizrahman4u any one do the code review? |
|
LGTM, need confirmation from one more moderator. |
An Implementation of linear chain conditional random field (CRF), modified for keras 2.0 from previous PR keras-team/keras#4646
Note this PR has #79 merged. Please merged.