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

Alignment CRF error #106

Open
jdegange opened this issue Sep 23, 2021 · 6 comments
Open

Alignment CRF error #106

jdegange opened this issue Sep 23, 2021 · 6 comments

Comments

@jdegange
Copy link

jdegange commented Sep 23, 2021

Looks like there is an error now on install for AlignmentCRF.

https://github.com/harvardnlp/pytorch-struct/blob/master/notebooks/CTC_with_padding.ipynb

Find marginals (see uncertainty from randomness)
show(dist.marginals, 1)

Error:
`
~/opt/anaconda3/lib/python3.7/site-packages/torch_struct/alignment.py in dp_scan(self, log_potentials, lengths, force_grad)
98 point = (l + M) // 2
99
--> 100 charta[1][:, b, point:, 1, ind, :, :, Mid] = semiring.one
(
101 charta[1][:, b, point:, 1, ind, :, :, Mid]
102 )

AttributeError: type object 'LogSemiring' has no attribute 'one_'
`

@JohnReid
Copy link
Contributor

Turns out the one_ method was removed (in #105 I think). You can change the calling code on line 100 in torch_struct/alignment.py to what the method used to do:

-            charta[1][:, b, point:, 1, ind, :, :, Mid] = semiring.one_(
-                charta[1][:, b, point:, 1, ind, :, :, Mid]
-            )
+            # semiring.one_() was removed
+            charta[1][:, b, point:, 1, ind, :, :, Mid] = charta[1][:, b, point:, 1, ind, :, :, Mid].fill_(0)

However despite this progress I still have problems when calculating the marginals using genbmm as my tensors are not cuda.

@JohnReid
Copy link
Contributor

@srush would it be worth adding a test for AlignmentCRF or Alignment that would have failed for #105? I'm happy to send a PR.

@srush
Copy link
Collaborator

srush commented Sep 30, 2021

Oh, Unfortunately alignment crf only runs on gpu, so the tests don't catch this.

Are you using it? Wondering if I should just remove it until I figure out a better approach.

@JohnReid
Copy link
Contributor

I was hoping to use it. Can the tests not run on the GPU or am I missing the point?

@srush
Copy link
Collaborator

srush commented Sep 30, 2021

Yes, they should run on GPU, but they don't run automatically. If you want to fix them and check that they pass that would be great. Otherwise, I'll take a look (but likely not until next week).

@JohnReid
Copy link
Contributor

JohnReid commented Oct 1, 2021

See #109. I'm not 100% sure this is what you suggested I do but it is a starting point at least.

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

No branches or pull requests

3 participants