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

Refactor span extractors and unify forward. #5160

Merged
merged 7 commits into from
May 3, 2021
Merged

Refactor span extractors and unify forward. #5160

merged 7 commits into from
May 3, 2021

Conversation

izhx
Copy link
Contributor

@izhx izhx commented Apr 27, 2021 •
edited
Loading

Fixes #5145. Thanks! @dirkgr

Changes proposed in this pull request:

  • added a SpanExtractorWithSpanWidthEmbedding class with __init__, forward and get_input_dim methods, putting specific span embedding computations into the abstract mehtod _embed_spans.
  • added the __init__ method to SpanExtractor for initiating span_width_embedding and other attributes.
  • added the forward method to SpanExtractor to unify forward arguments of all extractors.
  • added comments for width = span_end - span_start.
  • added an _embed_spans method, which need to be implemented to compute span embeddings in different ways, to SpanExtractor.
  • modified EndpointSpanExtractor, SelfAttentiveSpanExtractor and BidirectionalEndpointSpanExtractor accordingly (change forward to _embed_spans, Inherit from SpanExtractorWithSpanWidthEmbedding).

Now, SelfAttentiveSpanExtractor can also embed span widths.

ArjunSubramonian
ArjunSubramonian reviewed Apr 27, 2021
View reviewed changes
Copy link
Contributor

@ArjunSubramonian ArjunSubramonian left a comment

Choose a reason for hiding this comment

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

This personally looks great to me! Thanks for refactoring. I'm adding @dirkgr to look it over as well.

@ArjunSubramonian ArjunSubramonian requested a review from dirkgr April 27, 2021 17:37
dirkgr
dirkgr suggested changes Apr 27, 2021
View reviewed changes
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.

I would like to keep SpanExtractor as a purely abstract base class, just in case someone wants to build a totally different span extractor. But it makes sense to have a SpanExtractorWithSpanWidthEmbedding class that's basically the class you wrote, and have all the concrete implementations derive from that (the way you already have it). Does that make sense?

Thanks for finding and fixing this!

allennlp/modules/span_extractors/span_extractor.py Outdated
)
if self._span_width_embedding is not None:
# width = end_index - start_index + 1 since `SpanField` use inclusive indices.
# But here we do not add 1 beacuse we offen initiate the span width
Copy link
Member

Choose a reason for hiding this comment

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

Typo

Copy link
Contributor Author

@izhx izhx Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Changed offen to often. 🤣

@izhx
Copy link
Contributor Author

izhx commented Apr 28, 2021

I would like to keep SpanExtractor as a purely abstract base class, just in case someone wants to build a totally different span extractor. But it makes sense to have a SpanExtractorWithSpanWidthEmbedding class that's basically the class you wrote, and have all the concrete implementations derive from that (the way you already have it). Does that make sense?

Thanks for finding and fixing this!

Yeah, I totally agree with you! It's going to be flexible for future developments.
I have made these changes, Thanks!

@ArjunSubramonian
Copy link
Contributor

I would like to keep SpanExtractor as a purely abstract base class, just in case someone wants to build a totally different span extractor. But it makes sense to have a SpanExtractorWithSpanWidthEmbedding class that's basically the class you wrote, and have all the concrete implementations derive from that (the way you already have it). Does that make sense?
Thanks for finding and fixing this!

Yeah, I totally agree with you! It's going to be flexible for future developments.
I have made these changes, Thanks!

@izhx Thanks for your great work! The changes look fine to me. I would edit the description of the PR to reflect what you reverted. Also, as a heads up, @dirkgr is out the rest of this week, so please give us a week to get back to you.

@izhx
Copy link
Contributor Author

izhx commented Apr 29, 2021

I would like to keep SpanExtractor as a purely abstract base class, just in case someone wants to build a totally different span extractor. But it makes sense to have a SpanExtractorWithSpanWidthEmbedding class that's basically the class you wrote, and have all the concrete implementations derive from that (the way you already have it). Does that make sense?
Thanks for finding and fixing this!

Yeah, I totally agree with you! It's going to be flexible for future developments.
I have made these changes, Thanks!

@izhx Thanks for your great work! The changes look fine to me. I would edit the description of the PR to reflect what you reverted. Also, as a heads up, @dirkgr is out the rest of this week, so please give us a week to get back to you.

Thank you! 😄

dirkgr
dirkgr approved these changes May 3, 2021
View reviewed changes
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.

Thank you!

@dirkgr dirkgr enabled auto-merge (squash) May 3, 2021 18:29
@dirkgr dirkgr merged commit b533733 into allenai:main May 3, 2021
@izhx izhx deleted the extractor branch May 4, 2021 17:23
dirkgr added a commit that referenced this pull request May 10, 2021
@izhx @dirkgr
Refactor span extractors and unify forward. (#5160)
6c6fe9a 
* Refactor span extractors

* add SpanExtractorWithSpanWidthEmbedding

* update changelog

* fix blank lines

Co-authored-by: Dirk Groeneveld <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ArjunSubramonian ArjunSubramonian ArjunSubramonian left review comments

@dirkgr dirkgr dirkgr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactoring span extractors to fix wrong span width calculation and unify forward arguments
3 participants
@izhx @ArjunSubramonian @dirkgr