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 experimental coref docs #11291

Merged
merged 17 commits into from
Sep 27, 2022
Merged

Conversation

polm
Copy link
Contributor

@polm polm commented Aug 10, 2022

Description

This PR includes just the docs written in #7264, which now cover the components in explosion/spacy-experimental#17. #7264 can be closed in favor of this PR.

This should not be merged before explosion/spacy-experimental#17.

One question is what version should be listed for the introduction of these components; currently it's 3.4.

(A few unrelated lines in the architecture docs were modified by prettier.)

Types of change

docs update

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 docs Documentation and website feat / coref Feature: Coreference resolution labels Aug 10, 2022
@svlandeg svlandeg mentioned this pull request Aug 11, 2022
3 tasks
@svlandeg
Copy link
Member

One question is what version should be listed for the introduction of these components; currently it's 3.4.

It shouldn't list any spaCy version yet, until we actually port it over from experimental.

@adrianeboyd
Copy link
Contributor

Are we sure we want to put this in the main docs? There's no explanation of spacy-experimental or tags related to this?

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, good idea to move this to a new PR for clarity :-)

Had a few more minor comments.

website/docs/api/coref.md Outdated Show resolved Hide resolved
website/docs/api/coref.md Outdated Show resolved Hide resolved
website/docs/api/coref.md Outdated Show resolved Hide resolved
website/docs/api/coref.md Outdated Show resolved Hide resolved
website/docs/api/coref.md Outdated Show resolved Hide resolved
website/docs/api/coref.md Outdated Show resolved Hide resolved
polm and others added 3 commits August 15, 2022 14:17
Co-authored-by: Sofie Van Landeghem <svlandeg@users.noreply.github.com>
It seems a period after a number made this think it was a list?
@adrianeboyd
Copy link
Contributor

I think it would be okay to redefine the experimental tag to mean spacy-experimental, but we should then keep it that way in the future rather than trying to use it for both uses.

@polm
Copy link
Contributor Author

polm commented Aug 16, 2022

I think it would be okay to redefine the experimental tag to mean spacy-experimental, but we should then keep it that way in the future rather than trying to use it for both uses.

In that case what should we do for features like rehearsal, that can't be split out into spacy-experimental?

@adrianeboyd
Copy link
Contributor

That is a good point. I think having two separate experimental and spacy-experimental tags is going to be too confusing.

@polm
Copy link
Contributor Author

polm commented Aug 16, 2022

How about we:

  1. use experimental for both spacy-experimental and experimental features in core
  2. make a point of using warning infoboxes or other techniques to signpost things in spacy-experimental?

I think the point of the experimental tag is to indicate that APIs may be unstable or it may have seen less real-world use, and that's true of features in core or spacy-experimental, so sharing the tag makes sense.

While it is true that it can be confusing if someone misses the note that an experimental feature is in a separate package, I think that's the kind of temporary confusion that's easily fixed, rather than something more serious like breaking an existing application. If we're diligent about signposting things that require the extra library, then I think the small chance of confusion is a fair tradeoff for the benefit of having the docs all in one place with consistent formatting (and being searchable too).

@polm
Copy link
Contributor Author

polm commented Aug 17, 2022

I'll put this in draft state until explosion/spacy-experimental#17 has been merged.

@polm polm marked this pull request as draft August 17, 2022 09:35
@polm
Copy link
Contributor Author

polm commented Sep 14, 2022

Test failure here is due to python/mypy#13627. Since this PR contains no code, only docs, it should be safe to ignore.

website/docs/api/architectures.md Outdated Show resolved Hide resolved
website/docs/api/architectures.md Outdated Show resolved Hide resolved
website/docs/api/coref.md Outdated Show resolved Hide resolved
website/docs/api/coref.md Outdated Show resolved Hide resolved
website/docs/api/span-resolver.md Outdated Show resolved Hide resolved
website/docs/api/span-resolver.md Outdated Show resolved Hide resolved
website/docs/api/span-resolver.md Outdated Show resolved Hide resolved
These weren't properly updated when the code was moved out of spacy
core.
@polm
Copy link
Contributor Author

polm commented Sep 15, 2022

It looks like this hadn't actually been updated to make sure the sample code worked since the implementation moved to spacy-experimental, but that should be taken care of now.

@svlandeg svlandeg marked this pull request as ready for review September 15, 2022 14:03
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.

Found two more outdated spots:

website/docs/api/architectures.md Outdated Show resolved Hide resolved
website/docs/api/architectures.md Outdated Show resolved Hide resolved
@polm polm merged commit a44b7d4 into explosion:master Sep 27, 2022
polm added a commit that referenced this pull request Sep 27, 2022
* Add experimental coref docs

* Docs cleanup

* Apply suggestions from code review

Co-authored-by: Sofie Van Landeghem <svlandeg@users.noreply.github.com>

* Apply changes from code review

* Fix prettier formatting

It seems a period after a number made this think it was a list?

* Update docs on examples for initialize

* Add docs for coref scorers

* Remove 3.4 notes from coref

There won't be a "new" tag until it's in core.

* Add docs for span cleaner

* Fix docs

* Fix docs to match spacy-experimental

These weren't properly updated when the code was moved out of spacy
core.

* More doc fixes

* Formatting

* Update architectures

* Fix links

* Fix another link

Co-authored-by: Sofie Van Landeghem <svlandeg@users.noreply.github.com>
Co-authored-by: svlandeg <svlandeg@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation and website feat / coref Feature: Coreference resolution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants