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

Raise error if deps not provided with heads #8335

Merged
merged 10 commits into from Jun 15, 2021

Conversation

polm
Copy link
Contributor

@polm polm commented Jun 10, 2021

Before this change, if heads were passed without deps they would be
silently ignored, which could be confusing. See #8334.

Description

Types of change

Minor bugfix

Checklist

  • I have submitted the spaCy Contributor Agreement.
  • 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.

Before this change, if heads were passed without deps they would be
silently ignored, which could be confusing. See explosion#8334.
@polm polm added bug Bugs and behaviour differing from documentation docs Documentation and website labels Jun 10, 2021
@adrianeboyd adrianeboyd added feat / doc Feature: Doc, Span and Token objects and removed docs Documentation and website labels Jun 10, 2021
@adrianeboyd
Copy link
Contributor

It would be better to use a non-empty string here, and we normally use "dep" for a placeholder non-empty dep.

The problem with the head attribute is that there's no value for a missing head, so in Doc.has_annotation and other places, whether the dep is set is used to decide whether both heads and deps are set. So even if this works for the Doc creation (and I'm not sure it does in the end?), in other places it still won't look like there's a dependency parse.

This is the customary placeholder dep. It might be better to show an
error here instead though.
spacy/tokens/doc.pyx Outdated Show resolved Hide resolved
@adrianeboyd
Copy link
Contributor

I think an error as Matt suggested would be fine as an alternative to this. You could give a suggestion about how to fill in default deps as deps=["dep"]*len(heads) or something like that in the error message.

@polm
Copy link
Contributor Author

polm commented Jun 12, 2021

Huh, turns out there's more than few tests that pass heads without deps.

spacy/errors.py Outdated Show resolved Hide resolved
@adrianeboyd
Copy link
Contributor

Looks fine, I will just do some minor whitespace cleanup.

@adrianeboyd adrianeboyd changed the title Fill in deps if not provided with heads Raise error if deps not provided with heads Jun 15, 2021
@adrianeboyd adrianeboyd merged commit 2c105cd into explosion:master Jun 15, 2021
adrianeboyd added a commit to adrianeboyd/spaCy that referenced this pull request Jul 16, 2021
* Fill in deps if not provided with heads

Before this change, if heads were passed without deps they would be
silently ignored, which could be confusing. See explosion#8334.

* Use "dep" instead of a blank string

This is the customary placeholder dep. It might be better to show an
error here instead though.

* Throw error on heads without deps

* Add a test

* Fix tests

* Formatting

* Fix all tests

* Fix a test I missed

* Revise error message

* Clean up whitespace

Co-authored-by: Adriane Boyd <adrianeboyd@gmail.com>
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 / doc Feature: Doc, Span and Token objects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants