8000 Bugfix: initializing all tensors and parameters of the `ConditionalRandomField` model on the proper device by oroszgy · Pull Request #5335 · allenai/allennlp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Bugfix: initializing all tensors and parameters of the ConditionalRandomField model on the proper device #5335

Merged
merged 6 commits into from
Aug 18, 2021

Conversation

oroszgy
Copy link
Contributor
@oroszgy oroszgy commented Jul 27, 2021

Bugfix: crf_tagger was doing most of the computations on the CPU, as the ConditionalRandomField model's parameters and some necessary tensors were initialized on the wrong device.

Fixes #2884 .

Changes proposed in this pull request:

  • ConditionalRandomField's transitions and tag_sequence tensors are initialized on the proper device

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

After submitting

  • All GitHub Actions jobs for my pull request have passed.
  • codecov/patch reports high test coverage (at least 90%).
    You can find this under the "Actions" tab of the pull request once the other checks have finished.

@AkshitaB AkshitaB self-requested a review July 30, 2021 22:13
Copy link
Member
@dirkgr dirkgr left a comment

Choose a reason for hiding this comment

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

Just one little change, and this is ready to go!

@@ -364,7 +364,7 @@ def viterbi_tags(
# Augment transitions matrix with start and end transitions
start_tag = num_tags
end_tag = num_tags + 1
transitions = torch.Tensor(num_tags + 2, num_tags + 2).fill_(-10000.0)
transitions = torch.empty(num_tags + 2, num_tags + 2, device=logits.device).fill_(-10000.0)
Copy link
Member

Choose a reason for hiding this comment

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

This should use torch.full(), so we can create the tensor in one go.

Copy link
Member

Choose a reason for hiding this comment

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

It will also be faster!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: 84d4e20

@dirkgr
Copy link
Member
dirkgr commented Aug 18, 2021

Ah, it also needs an entry in the changelog. So two little changes I guess.

@oroszgy
Copy link
Contributor Author
oroszgy commented Aug 18, 2021

Ah, it also needs an entry in the changelog. So two little changes I guess.

@dirkgr Done: 7ffd9ae

@dirkgr dirkgr enabled auto-merge (squash) August 18, 2021 18:52
@dirkgr
Copy link
Member
dirkgr commented Aug 18, 2021

Thank you!

@dirkgr dirkgr merged commit bffdbfd into allenai:main Aug 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why CRF lead a high cost on CPU?
4 participants
0