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

Add test for case where parser overwrite annotations #9406

Merged
merged 3 commits into from
Oct 11, 2021

Conversation

polm
Copy link
Contributor

@polm polm commented Oct 9, 2021

Description

As pointed out in #7716, the parser can overwrite is_sent_start annotations if not all tokens in a doc have them. The exact circumstances for this are unclear, but specifically it can turn False annotations into True.

This just adds a failing test for that case. The bad news is the test is flaky and succeeds occasionally.

It might make sense to leave this PR around until a fix can be added to it, or to change the test to a non-strict xfail instead.

Types of change

Adds a test.

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

@polm polm added bug Bugs and behaviour differing from documentation feat / parser Feature: Dependency Parser labels Oct 9, 2021
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Thanks!

I would move this test to tests/regression/test_issue7716.py. Anyone looking to fix the original issue should be writing the test at that spot, and should thus find this. And ofcourse the original issue is now linked to this PR so the trail is visible. But in general if there is an associated issue, we prefer having it as part of the regression test suite.

@svlandeg svlandeg marked this pull request as draft October 11, 2021 09:39
polm added 2 commits October 11, 2021 20:05
Also add note about how other tokens modify results.
@svlandeg svlandeg marked this pull request as ready for review October 11, 2021 12:56
@svlandeg svlandeg merged commit efe5bee into explosion:master Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs and behaviour differing from documentation feat / parser Feature: Dependency Parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants