Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Add hierplane to the SRL output as a vizualization method. #526

Merged
merged 8 commits into from
Nov 22, 2017

Conversation

codeviking
Copy link
Contributor

  • Adds a hierplane visualization to the SRL output, where each
    verb is rendered as it's own tree.
  • Add UI controls that allow the user to toggle between the "table" and
    "tree" view.
  • Ignore node_modules, as it's a generated folder.

@schmmd Please review, or determine who is appropriate to review.

* Adds a `hierplane` visualization to the SRL output, where each
  verb is rendered as it's own tree.
* Add UI controls that allow the user to toggle between the "table" and
  "tree" view.
* Ignore `node_modules`, as it's a generated folder.
* Fix the container, so it takes up all available height.
* Fix the active tab indicator so that it's in the right spot.
@codeviking
Copy link
Contributor Author

An obligatory screenshot:

image

Copy link
Contributor

@DeNeutoy DeNeutoy left a comment

Choose a reason for hiding this comment

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

This looks so awesome 👍

children: tags.reduce((children, tag, idx) => {
if (tag !== 'B-V' && tag.startsWith('B-')) {
const word = response.words[idx];
const link = tag.split('-').pop().replace(/\d+/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

The numeric characters in the arguments actually carry some meaning for SRL - can you leave them in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, for sure!

word,
spans: [{
start,
end: start + word.length + 1
Copy link
Contributor

@DeNeutoy DeNeutoy Nov 22, 2017

Choose a reason for hiding this comment

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

What would be really nice is if we could make the nodeType and attributes different.
The SRL labels either contain ARGM, which modify the verb, or ARG1-5 which are the core arguments. For the ARGM, the nodeType could be modifier and for the ARG1-5 it could be arg, with the attribute containing the second half of the label, eg TMP or DIS.

One final point to be aware of is that some of the labels might contain C-ARG1 or R-ARG-1, which mean that the label is continued or referred to, eg (The place ARG1), beautiful though it was, (where R-ARG1) Mark lived.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes a ton of sense! I can easily make this happen. I'll make the change and get a local demo up today so you can review it / play with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the PRP suffix of value, or something I can disregard for this purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, PRP is likely the "second half" -- I think I grok things now.

Would R- and C- both be appropriately labeled as a reference, or are they different types?

Copy link
Contributor

Choose a reason for hiding this comment

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

R would be reference and C would be continuation, they are very rarely observed but can crop up so we should handle them.

PRP is actually the label, it is short hand for purpose, i.e why the verb is being done. I'm not exactly sure what you mean by second half, feel free to swing by and we can chat in person.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤘 Thx. By the "second half" I mean what you mentioned here:

with the attribute containing the second half of the label, eg TMP or DIS.

So I'm assuming TMP, DIS and PRP all serve similar purposes (pun intended!) :-D. LMK if that's not the case. Also I have an interactive version I can link to shortly.

@schmmd
Copy link
Member

schmmd commented Nov 22, 2017

Hi Sam, looks fantastic. A few thoughts:

  • Why is "which" highlighted in the source sentence? Did the user click on it in the diagram?
  • I will probably make a subsequent PR that puts the diagram first, and changes the table view to a text output for SRL.

@codeviking
Copy link
Contributor Author

codeviking commented Nov 22, 2017

@schmmd 🤘

Why is "which" highlighted in the source sentence? Did the user click on it in the diagram?

I was hovering it (I wanted to show that case). I wish screenshots captured the cursor :-D.

I will probably make a subsequent PR that puts the diagram first, and changes the table view to a text output for SRL.

Sounds good. I've got some things to tweak, do you want me to make the default switch as part of that so you don't have to?

@schmmd
Copy link
Member

schmmd commented Nov 22, 2017

Sure, you're welcome to switch the tab order.

* Make the passage resize to fit the text.
* Set a max-width for nodes so they don't get too big.
* Adjust parsing to handle references, continuations and `ARGA`
  nodes as appropriate.
* Convert attributes to a friendly display value.
@codeviking
Copy link
Contributor Author

Alright @schmmd and @DeNeutoy I've incorporated your feedback. Things are looking pretty good. Can one of you guys take another pass and green-stamp it if it looks good?

Also, once it's merged, can one of you help me figure out how to deploy it? Or is CD in place such that it'll automatically go live?

@schmmd
Copy link
Member

schmmd commented Nov 22, 2017

@codeviking looks great.

Deploys are broken presently. I'm running the AllenNLP demo from a single AWS instance ever since Kubernetes went down. I will use TeamCity (http://allennlp-build.dev.ai2:8111/) to deploy it to GKE. We should be able to switch our DNS entries to GKE after the break.

@codeviking
Copy link
Contributor Author

Cool, I can't merge until it's approved by someone with write access:

image

So if it looks good to go, that green stamp will let me throw this into master!

@schmmd schmmd self-requested a review November 22, 2017 21:54
@schmmd schmmd merged commit 75c627f into allenai:master Nov 22, 2017
gabrielStanovsky pushed a commit to gabrielStanovsky/allennlp that referenced this pull request Sep 7, 2018
…#526)

* Add `hierplane` to the SRL output as a vizualization method.

* Adds a `hierplane` visualization to the SRL output, where each
  verb is rendered as it's own tree.

* Add UI controls that allow the user to toggle between the "table" and
  "tree" view.
* Ignore `node_modules`, as it's a generated folder.

* Fix the container, so it takes up all available height.

* Fix the active tab indicator so that it's in the right spot.

* Don't select the label, it makes it harder to use.

* Polish.

* Make the passage resize to fit the text.
* Set a max-width for nodes so they don't get too big.

* Make the tree the default viz.

* Parsing adjustments; Display labels.

* Adjust parsing to handle references, continuations and `ARGA`
  nodes as appropriate.

* Convert attributes to a friendly display value.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants