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

Correct alignment example and documentation #11491

Merged
merged 4 commits into from
Sep 14, 2022

Conversation

richardpaulhudson
Copy link
Contributor

Description

Closes #11482.

Types of change

Correction to the documentation, including an example that was not running correctly

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.

@svlandeg svlandeg added the docs Documentation and website label Sep 13, 2022
@shadeMe
Copy link
Contributor

shadeMe commented Sep 13, 2022

Could you also update the outdated references to Ragged in spaCy/website/docs/api/example.md?

@richardpaulhudson richardpaulhudson marked this pull request as draft September 13, 2022 11:40
@richardpaulhudson
Copy link
Contributor Author

richardpaulhudson commented Sep 13, 2022

Converted to draft as it turns out this is another aspect to this task I hadn't been aware of.

@@ -286,10 +286,12 @@ Calculate alignment tables between two tokenizations.

### Alignment attributes {#alignment-attributes"}

Alignment attributes are managed using the `AlignmentArray`, which is a simplified version of Thinc's [Ragged](https://thinc.ai/docs/api-types#ragged) type that only supports the `data` and `length` attributes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a separate container entry for AlignmentArray seemed like overkill, so I just added this note instead. Although it closely follows the in-code documentation, it's not 100% accurate as AlignmentArray actually contains two additional properties, but none that a user would be likely to want to access. I feel this solution is a good compromise between accuracy and clarity/conciseness.

@richardpaulhudson richardpaulhudson marked this pull request as ready for review September 13, 2022 12:09
Copy link
Contributor

@shadeMe shadeMe left a comment

Choose a reason for hiding this comment

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

Looks good! The example still contains a few references to dataXd, though (lines 314-317).

@richardpaulhudson
Copy link
Contributor Author

Thanks, I hadn't seen those!

Copy link
Contributor

@shadeMe shadeMe left a comment

Choose a reason for hiding this comment

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

LGTM! Just one minor comment.

Co-authored-by: Madeesh Kannan <shadeMe@users.noreply.github.com>
@shadeMe shadeMe merged commit 3f0c3ad into explosion:master Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation and website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alignment API has changed, documentation is incorrect.
3 participants