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

Issue 665 #687

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Issue 665 #687

wants to merge 5 commits into from

Conversation

ChaseDuncan
Copy link
Contributor

@ChaseDuncan ChaseDuncan commented Aug 30, 2018

  • Changed SpanLabelView.addConstituent(Constituent) to enforce nonoverlapping
    constituents when appropriate, i.e. the flag is set.
  • Added a class with two basic tests to make sure that the changes work
    correctly.

Closes #665

- Changed SpanLabelView.addConstituent(Constituent) to enforce nonoverlapping
  constituents when appropriate, i.e. the flag is set.
- Added a class with two basic tests to make sure that the changes work
  correctly.
Copy link
Contributor

@mssammon mssammon left a comment

Choose a reason for hiding this comment

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

The test looks great. Very minor changes requested.

@@ -60,6 +58,13 @@ public SpanLabelView(String viewName, String viewGenerator, TextAnnotation text,

@Override
public void addConstituent(Constituent constituent) {

Copy link
Contributor

Choose a reason for hiding this comment

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

small improvement: please move start/end assignments inside if{} block, as they aren't being used otherwise.

import java.util.List;

public class SpanLabelViewTest {
// test that addConstituent(Constituent) does not allow overlapping spans
Copy link
Contributor

Choose a reason for hiding this comment

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

make the inline comment a class javadoc. comment can be exactly the same.

Copy link
Member

@mayhewsw mayhewsw left a comment

Choose a reason for hiding this comment

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

Wasn't this functionality already there? It's not clear what has changed.

@mssammon
Copy link
Contributor

@mayhewsw , there were ways to add constituents that sidestepped the overlap check. This issue is intended to fix the problem.

@mayhewsw
Copy link
Member

Ah, I see it now. Yes, looks good.

- Moved inline comment into JavaDoc comment above class in SpanLabelViewTest
- Moved start, end variable initialization and overlap into conditional
  statement so that they only occur when overlapping spans are not allowed
- Moved SpanLevelViewTest into correct module
Copy link

@cogcomp-dev cogcomp-dev left a comment

Choose a reason for hiding this comment

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

LGTM.

@cogcomp-dev
Copy link

@ChaseDuncan one test is failing in CI (teamcity build): please check into it and comment here if it is not related to the changes you made.

@danyaljj
Copy link
Member

danyaljj commented Sep 4, 2018

Copy link
Contributor

@mssammon mssammon left a comment

Choose a reason for hiding this comment

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

LGTM.

- Fixed multiple places in MD and one place in corpus readers where
  overlapping spans were implicitly or explicitly forbidden but still
  being used
@ChaseDuncan
Copy link
Contributor Author

It looks like there are more places where this was being done unintentionally. Will look into it...

- Added another ctor to TokenLabelView which has a parameter
  for specifying whether or not to allow overlapping spans
- Changed the TokenLabelView ctor which is used in StanfordTrueCaseHandler
  since it was clear that overlapping spans are desired
@mssammon
Copy link
Contributor

@ChaseDuncan looks like CI build is failing -- please take a look...

@ChaseDuncan
Copy link
Contributor Author

ChaseDuncan commented Sep 24, 2018

@mssammon I think the problem is a conflict in the design of mention detection and the some of the other CCG software like BasicAnnotatorService.

For example:

By default MD appears to create overlapping spans, e.g.
[NAM-ORG the CDC ] [NOM-PER the CDC 's Dr. ] [NAM-PER the CDC 's Dr. Michael Jhung ] .

For some (probably good) reason I don't understand, there's some View copying happening in BasicAnnotatorService. By default it creates Views which do not allow overlapping spans.
So when it tries to copy the constituents from the MENTION View into a new View instance, it dies.
Previously this was done using addConstituent(Constituent) so the allowOverlappingSpans check was circumvented.

It's not immediately clear to me how best to address this since the problem involves a bunch of complex system for which I have basically no knowledge. But, if no one else knows what to do, I can look into the code and see what I can figure out.

@mssammon
Copy link
Contributor

I think it is reasonable to change any code that violates the assumption by changing it to explicitly allow overlapping constituents -- since that is going on anyway. For posterity, you could create a list of any such places you change, and an issue that notifies everyone these were changed -- that would ensure (in theory) that someone checks each one. If you want to make this your issue for the next k weeks, please go ahead and then ask anyone who seems likely to have a clue for each given component. @Slash0BZ and @danyaljj are likely candidates for who to ask w.r.t. the Mention view.

@Slash0BZ
Copy link
Member

Ah I didn't know that SpanLabelView by design discourages overlapped mentions. In MD, complete mention spans can overlap with each other, but mention heads cannot. I think we either need a special case for SpanLabelView that allows overlapped mentions, or I can try to only add mention heads to the view, and use separate function to generate the complete mention using information stored in the head.

@mssammon
Copy link
Contributor

@Slash0BZ I think the mention view should allow overlap, and that you should provide a utility method in the relevant code that verifies no constraints are violated.

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

Successfully merging this pull request may close these issues.

6 participants