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

Constituency js #916

Merged
merged 7 commits into from
Feb 24, 2018
Merged

Constituency js #916

merged 7 commits into from
Feb 24, 2018

Conversation

DeNeutoy
Copy link
Contributor

@DeNeutoy DeNeutoy commented Feb 23, 2018

  • Adds a Hierplane demo for the constituency parser.
  • Weaves a new bit of metadata through the model so that we don't get unknown words in the demo (and actually, simplified things quite a bit)
  • I haven't added the model yet, because it's not good enough and I still need to figure out why. But merging this doesn't depend on that, so I figured it would be best to split it up here.

The demo looks like this:
(We've hit maximum text wrapping capacity on a mac on the demo - we're going to need a "Select a demo model" page pretty soon.... it could look like this but for deep learning models)
image

@@ -47,7 +47,8 @@ def test_read_from_file(self):
"VP(TO to)(VP(VB be)(ADJP(JJ fair)(PP(TO to)(NP(JJ other)(NNS "
"bidders))))))))))))))(. .)))")

assert fields["gold_tree"].metadata == gold_tree
assert fields["metadata"].metadata["gold_tree"] == gold_tree
assert fields["metadata"].metadata["tokens"] == tokens
Copy link
Contributor

Choose a reason for hiding this comment

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

this always feels clunky to me, I wonder if MetadataField should implement __getitem__? (not in this PR of course, just in general)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do this for all of our fields, e.g. for token in TextField etc, etc. I'm a fan

Copy link
Contributor

Choose a reason for hiding this comment

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

What does for token in TextField return, and when is it called?

For the MetadataField, it was originally a hack to get something working for BiDAF. There is definitely room to improve 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.

https://github.com/allenai/allennlp/blob/master/tests/data/dataset_readers/penn_tree_bank_reader_test.py#L24

it would be equivalent to that list comprehension (or probably returning Tokens). Maybe it's less useful because it's unclear what it would return, but it would make it much cleaner.

@DeNeutoy DeNeutoy merged commit 8b706e4 into allenai:master Feb 24, 2018
@DeNeutoy DeNeutoy deleted the constituency-js branch February 24, 2018 00:27
gabrielStanovsky pushed a commit to gabrielStanovsky/allennlp that referenced this pull request Sep 7, 2018
* 'working' constituency parsing demo

* wire the actual sentence through model, predictor and dataset reader

* 90% working hierplane vis

* tidy up, remove sentence lengths from return dict

* clean up some js, remove spans from the demo

* better description of the model

* add explicit hierplane tree in test
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.

3 participants