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

Add conditional random fields #314

Merged
merged 11 commits into from
Aug 1, 2019
Merged

Add conditional random fields #314

merged 11 commits into from
Aug 1, 2019

Conversation

Squadrick
Copy link
Member

@Squadrick Squadrick commented Jun 21, 2019

WIP till I can resolve some of the failing test cases.
Closes #22

Ref: tf.contrib.crf

@Squadrick Squadrick added text wip Work in-progress kokoro:force-run and removed wip Work in-progress labels Jun 21, 2019
@Squadrick Squadrick changed the title [WIP] Add conditional random fields Add conditional random fields Jun 24, 2019
@Squadrick
Copy link
Member Author

The test cases were failing due to not masking the inputs to the RNN.

@Squadrick
Copy link
Member Author

Ping @seanpmorgan @facaiy

Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this! Some initial comments, and then I'll get around to a more thorough review this week.

One thing that would be nice after this merges would be an example colab notebook as shown in the old README (but we could probably show more):
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/contrib/crf/README.md

tensorflow_addons/text/crf_ops.py Outdated Show resolved Hide resolved
tensorflow_addons/text/crf_ops.py Outdated Show resolved Hide resolved
tensorflow_addons/text/BUILD Outdated Show resolved Hide resolved
tensorflow_addons/text/crf_ops.py Outdated Show resolved Hide resolved
@facaiy
Copy link
Member

facaiy commented Jun 25, 2019

I don't know it. @qlzh727 Qianli, are you a good person to look at this?

cc @WindQAQ @howl-anderson @NLP-ZY who might be interested.

@facaiy facaiy removed their request for review June 25, 2019 05:28
@Squadrick
Copy link
Member Author

@seanpmorgan I'm making a colab notebook with usage right now, I'll open a separate PR once this merges.

@howl-anderson
Copy link
Contributor

Since TF 2.0 encourages users using Keras API, implement CRF functions in the form of Keras layer will be a great feature. keras-contrib already has a CRF layer (https://github.com/keras-team/keras-contrib/blob/master/keras_contrib/layers/crf.py) which is pretty widely used in Keras community. We'd better consistent with it.

@howl-anderson
Copy link
Contributor

I am working on implementing the CRF Keras layer based on Squadrick's solution. When it's ready I will post a new PR.

@Squadrick
Copy link
Member Author

@howl-anderson Will you using any of the functions implemented here? Similar to how tf.keras.layers internally wraps tf.nn or will you doing the computation from scratch.

If it's the latter, I'll close this PR and we can proceed with our approach. If it's the latter, I'll keep this open.

P.S.: Implement your CRF layer under tfa.layers.

@Squadrick
Copy link
Member Author

IMO, it's best to include both. The raw functions under tfa.text for those who want low level control, and an end-to-end layer under tfa.layers for easy integration with the Keras API. What do you think? Standardising the naming scheme used in both will be an important aspect.

What do y'all think? Any thoughts on this? @seanpmorgan @WindQAQ @facaiy

@howl-anderson
Copy link
Contributor

@Squadrick CRF layer will be based on your functions (internally wraps your functions). This is a friendly feature for Keras users. low-level functions (which compatible with tf.contrib) for CRF are still very useful for advanced users and it's very important to users who migrate they code from 1.x to 2.0 and don't want to change their code on a large scale.

Copy link
Member

@qlzh727 qlzh727 left a comment

Choose a reason for hiding this comment

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

Haven't looked deep into the logic yet. Probably need to have some one compare the original implementation again this PR.

tensorflow_addons/text/crf.py Outdated Show resolved Hide resolved
tensorflow_addons/text/crf.py Outdated Show resolved Hide resolved
tensorflow_addons/text/crf.py Outdated Show resolved Hide resolved
tensorflow_addons/text/crf.py Outdated Show resolved Hide resolved
tensorflow_addons/text/crf.py Outdated Show resolved Hide resolved
tensorflow_addons/text/crf.py Outdated Show resolved Hide resolved
tensorflow_addons/text/crf.py Outdated Show resolved Hide resolved
tensorflow_addons/text/crf.py Outdated Show resolved Hide resolved
tensorflow_addons/text/crf.py Outdated Show resolved Hide resolved
@howl-anderson
Copy link
Contributor

Haven't looked deep into the logic yet. Probably need to have some one compare the original implementation again this PR.

@qlzh727 I have some experiences with the original implementation of CRF in the tf.contrib and familiar with how CRF works, I can check/compare this PR with the original one.

@Squadrick Squadrick closed this Jul 4, 2019
@Squadrick Squadrick reopened this Jul 4, 2019
@Squadrick
Copy link
Member Author

Squadrick commented Jul 5, 2019

@qlzh727 @howl-anderson @seanpmorgan Modified the CRF code to use tf.scan for decode backward and forward alpha calculation. This makes the code slightly faster as well.

decode_forward still uses an RNN cell, but I can rewrite it to use a tf.while_loop, but it ends up being far more complicated then just using an RNN cell.

Also, transition_params will be not tracked by Keras RNN.

@howl-anderson
Copy link
Contributor

@Squadrick Thanks for your update

@Squadrick
Copy link
Member Author

@qlzh727 @howl-anderson Any feedback on this?

@howl-anderson
Copy link
Contributor

@howl-anderson Don't worry, I am still working on it.

@gaojie-shumei
Copy link

you said the crf is in the tfa.text, but I not find it, are you sure that merge is successful??

@Squadrick
Copy link
Member Author

@howl-anderson @guillaumekln @seanpmorgan Made the changes, this should be good to merge.

@howl-anderson
Copy link
Contributor

@Squadrick Nice job!

@seanpmorgan
Copy link
Member

Looks great @Squadrick Thanks! Going to leave this open just for a bit to test #387

@seanpmorgan
Copy link
Member

Assuming this is some edge case because the sanity checks are working on other PRs. Merging. Thanks @Squadrick !

@seanpmorgan seanpmorgan merged commit 47e6877 into tensorflow:master Aug 1, 2019
@Squadrick
Copy link
Member Author

Haha, not sure what caused the sanity check to fail. Finally got this long running PR closed.

Thanks for the reviews, guys.

@lingvisa
Copy link

@howl-anderson did you have this implementation in the tf-addons in TF 2.0? like this one: https://github.com/keras-team/keras-contrib/blob/master/keras_contrib/layers/crf.py

@seanpmorgan
Copy link
Member

Hi @lingvisa , we do have support for CRF, but as it stands today they are only TF functions:
https://github.com/tensorflow/addons/blob/master/tensorflow_addons/text/crf.py

We have an open PR for wrapping these around a layer, please feel free to review and let us know if it meets your expectations. At a first glance I think the keras-contrib version is much simplier and probably benefits from not have CRF functions carried over from tf.contrib. I'll make a note in that PR to consider dropping CRF functions and adopting the keras-contrib version more or less as is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Conditional Random Field
10 participants