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

1st order cky implementation #83

Closed
wants to merge 3 commits into from

Conversation

teffland
Copy link

@teffland teffland commented Dec 5, 2020

Hi,

I'd like to contribute this implementation of a first-order cky-style crf with anchored rule potentials: $\phi[i,j,k,A,B,C] := \phi(A_{i,j} \rightarrow B_{i,k}, C{k+1,j})$.

I also added code to the _Struct class that allows calculating marginals even if input tensors don't require a gradient (i.e., after model.eval())

Please let me know if you'd like to see any changes.

Thanks,
Tom

Copy link
Collaborator

@srush srush left a comment

Choose a reason for hiding this comment

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

This looks great! Very excited to add.

Couple questions:

  • Reshape always worries me. Can those be views?
  • Repeat is almost never necessary. What operators is failing there?
  • I would prefer the sr.sum be replace with a view and then a single sum.

Were you able to run any tests? I know travis is broken (I need to move to github), but it would be great to have at least one test. For other algorithms I enumerate all examples and score. Can you try that?

@teffland
Copy link
Author

teffland commented Dec 7, 2020

Great, excited to contribute!

Re: your questions

  • Reshape: I can definitely try this with view. Just curious, why does reshape worry you? I thought reshape was just a more robust version of view that creates a contiguous copy in cases where view would fail.
  • Repeat: true, I can try without and it should work I think, I guess I just naturally tend towards implementations that go through all of the steps explicitly but I should get better about trusting broadcasting.
  • One sum: this is definitely doable, will make that change
  • I did write some tests locally in a notebook a while back. I can dig those up and incorporate them. By enumerate and score, you mean check that the dp partition matches the partition computed by explicitly enumerating all structures, correct?

Thanks for the feedback,
Tom

@srush
Copy link
Collaborator

srush commented Dec 7, 2020

I can help with testing. Yes, all the algorithms in torch struct test that the enumeration has the same marginals. Its helped catch a ton of bugs.

  • Reshape- maybe they changed this, earlier version caused problems.

@teffland
Copy link
Author

teffland commented Dec 7, 2020

Cool, help with the testing would nice but I can also take a crack at it later. When I get to it I'll also incorporate the suggested changes, (and will try it out with view).

@srush
Copy link
Collaborator

srush commented Jan 11, 2021

Hey sorry, I flaked on helping out here. Send me an email if you want to work on it more. I'm going to do some repo maintenance this week.

@teffland
Copy link
Author

Hey, no worries same here. Will send an email

@teffland
Copy link
Author

included in #93

@teffland teffland closed this Jan 20, 2021
@srush srush reopened this Jan 20, 2021
@srush srush closed this Jan 20, 2021
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.

2 participants