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 displaCy data structures to docs #12202

Closed

Conversation

thomashacker
Copy link
Contributor

Description

This PR adds a new section to the api/top-level#functions documentation for displaCy: Visualizer data structures. The section shows examples of the three data structures (Dependency Parsing, Entity Recognition, and Span Classification) and their types formatted as tables. To reduce cluttering, the nested dictionary types are formatted as a collapsable table.

The idea for this PR initially came from this discussion #10950
The types are based on this gist by @pmbaumgartner

Types of change

Documentation

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.

@thomashacker thomashacker added docs Documentation and website feat / visualizers Feature: Built-in displaCy and other visualizers labels Jan 30, 2023
@thomashacker thomashacker self-assigned this Jan 30, 2023
Copy link
Contributor

@pmbaumgartner pmbaumgartner left a comment

Choose a reason for hiding this comment

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

My in-line code comments are a little out of order so let me summarize them in a comment:

  • I don't think we should link back to the Usage docs since we provide those examples in the sidebar
  • Can we find a better way to make it clear the accordions have information about the sub-structures in each data structure?
  • We should be consistent with the descriptions for the values/attributes where they exist in the spaCy API.

@pmbaumgartner pmbaumgartner removed the docs Documentation and website label Jan 31, 2023
@thomashacker
Copy link
Contributor Author

thomashacker commented Feb 2, 2023

@pmbaumgartner What was the reason for removing the docs label from this PR? 😢

@pmbaumgartner pmbaumgartner added the docs Documentation and website label Feb 2, 2023
@pmbaumgartner
Copy link
Contributor

@pmbaumgartner What was the reason for removing the docs label from this PR? 😢

I must have misclicked or used a keyboard shortcut accidentally. Sorry!

@svlandeg
Copy link
Member

svlandeg commented Feb 8, 2023

@pmbaumgartner: is your "changes requested" review still valid? (as discussed internally we don't usually use this feature as typically we'd go by the open review comments to determine whether something requires changes or not, but because you used it earlier I want to double check with you and not just dismiss the review)

@pmbaumgartner pmbaumgartner dismissed their stale review February 8, 2023 16:03

Changes addressed / wrong review type

@pmbaumgartner
Copy link
Contributor

@pmbaumgartner: is your "changes requested" review still valid? (as discussed internally we don't usually use this feature as typically we'd go by the open review comments to determine whether something requires changes or not, but because you used it earlier I want to double check with you and not just dismiss the review)

I wasn't aware this was still blocking. I dismissed the review, but haven't had time to review the recent changes yet, so I'll look again today.

@pmbaumgartner
Copy link
Contributor

pmbaumgartner commented Feb 8, 2023

In the ENT example data structure, title has a value of None. Shouldn't its actual type be Optional[str] then? Same for Spans?

And it looks like title isn't a field on the deps data, only ents and spans, is that right? I think we might want to open an issue for that, if that's the case.

@thomashacker
Copy link
Contributor Author

  • Adjusted the type of the title key in the docs
  • Added tests that check displaCy.render with manual data
  • Implemented title key to dependency visualizations and adjusted docs

@pmbaumgartner
Copy link
Contributor

👌 love the tests. I don't have feedback on anything else at this time.

@svlandeg
Copy link
Member

Closing in favour of #12875 (PR redone to target master)

@svlandeg svlandeg closed this Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation and website feat / visualizers Feature: Built-in displaCy and other visualizers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants