Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Roll backbone #5229

Merged
merged 18 commits into from
May 28, 2021
Merged

Roll backbone #5229

merged 18 commits into from
May 28, 2021

Conversation

jacob-morrison
Copy link
Contributor

@jacob-morrison jacob-morrison commented May 27, 2021

Changes proposed in this pull request:

  • Rolling and unrolling dimensions in the VilBERT backbone

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 adds a new feature, I've added tests that sufficiently cover my new functionality here

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.

@jacob-morrison jacob-morrison marked this pull request as ready for review May 27, 2021 21:16
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 question: Why repeat_interleave()?

allennlp/modules/backbones/vilbert_backbone.py Outdated Show resolved Hide resolved
allennlp/modules/backbones/vilbert_backbone.py Outdated Show resolved Hide resolved
@dirkgr
Copy link
Member

dirkgr commented May 28, 2021

Let me know when this is ready to be looked at again?

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.

Looks good. I assume there is a corresponding change in the models repo somewhere that changes how your reader operates, correct?

@jacob-morrison
Copy link
Contributor Author

Looks good. I assume there is a corresponding change in the models repo somewhere that changes how your reader operates, correct?

Yup! I already changed my nlvr2 reader to work with these changes; I'm training that model again now and then I'll be able to send out that PR as well.

@jacob-morrison jacob-morrison merged commit 3d5799d into main May 28, 2021
@jacob-morrison jacob-morrison deleted the roll-backbone branch May 28, 2021 22:36
Abhishek-P pushed a commit to Abhishek-P/allennlp that referenced this pull request Aug 11, 2021
Adding support for inputs to the backbone with more than 3 dimensions
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.

2 participants