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

Max out span extractor #5520

Merged
merged 9 commits into from
Jan 5, 2022
Merged

Max out span extractor #5520

merged 9 commits into from
Jan 5, 2022

Conversation

MSLars
Copy link
Contributor

@MSLars MSLars commented Dec 20, 2021

Closes #5449.

Changes proposed in this pull request:

  • Added MaxPoolingSpanExtractor. This components represents spans via a max-pooling operation.

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
    I received an Error
    E RuntimeError: function '_has_torch_function' already has a docstring
    when I execute
pytest -v --cov allennlp.modules.span_extractors.max_pooling_span_extractor tests/modules/span_extractors/max_pooling_span_extractor_test.py 

to calculate the test coverage.

  • 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.

@epwalsh
Copy link
Member

epwalsh commented Dec 23, 2021

Hey @MSLars, thanks for taking this. Are you still having an issue running tests? Also, could you update the CHANGELOG when you get a chance?

@epwalsh epwalsh self-assigned this Dec 23, 2021
@MSLars
Copy link
Contributor Author

MSLars commented Jan 4, 2022

@epwalsh I updates the Changelog. I still have the issue with running

pytest -v --cov  allennlp.modules.span_extractors.max_pooling_span_extractor tests/modules/span_extractors/max_pooling_span_extractor_test.py

which leads to

ERROR tests/modules/span_extractors/max_pooling_span_extractor_test.py - RuntimeError: function '_has_torch_function' already has a docstring

But I seem to have the same problem for each test with import torch.

When I execute

pytest -v --cov  allennlp tests

The same test as before runs without errors. So I guess its an issue with my development configuration and the code should be fine.

Copy link
Member

@epwalsh epwalsh 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, @MSLars, thank you! I just left a few minor comments.

Copy link
Member

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @MSLars!

@epwalsh epwalsh merged commit a3d7125 into allenai:main Jan 5, 2022
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.

Max Pooling Entity Extractor
2 participants