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

Adding the wikitables parser #1114

Merged
merged 2 commits into from Apr 21, 2018

Conversation

@matt-gardner
Copy link
Member

commented Apr 20, 2018

Performance of the parser still doesn't match the original model, but it should just be minor tweaks from here. It'll be a lot easier to manage git if this is merged into master now.

I didn't really change anything in the code, so this is just bringing things over from the wikitables branch, and it has all already been reviewed.

There are a few minor changes to trainer.py that I don't think got a close enough look previously, and so @DeNeutoy, if you want to give the changes I made there a look, that'd be great.

@matt-gardner matt-gardner requested a review from DeNeutoy Apr 20, 2018

@DeNeutoy
Copy link
Contributor

left a comment

LGTM


loss = self._batch_loss(batch, for_training=False)
val_loss += loss.data.cpu().numpy()
if loss is not None:
# You shouldn't necessarily have to compute a loss for validation, so we allow for

This comment has been minimized.

Copy link
@DeNeutoy

DeNeutoy Apr 21, 2018

Contributor

Actually when is this used?

This comment has been minimized.

Copy link
@matt-gardner

matt-gardner Apr 21, 2018

Author Member

This is used by the wikitables parser (which is why it's part of this PR). Our training loss is maximum marginal likelihood, but we don't have a set of candidate logical forms for all of the validation instances. We still can calculate accuracy, though, and we want to include all of the instances in the validation set, to get accurate metrics. So we need the training code to not crash when we're evaluating instances at validation time for which we can't actually compute a loss.

This comment has been minimized.

Copy link
@DeNeutoy

DeNeutoy Apr 21, 2018

Contributor

makes sense 👍

@matt-gardner matt-gardner merged commit 0df8978 into allenai:master Apr 21, 2018

3 checks passed

Pull Requests (AllenNLP) TeamCity build finished
Details
codecov/patch 90% of diff hit (target 90%)
Details
codecov/project 92% (-1%) compared to f7b736c
Details

@matt-gardner matt-gardner deleted the matt-gardner:add_wikitables_parser branch Apr 21, 2018

gabrielStanovsky pushed a commit to gabrielStanovsky/allennlp that referenced this pull request Sep 7, 2018
Adding the wikitables parser (allenai#1114)
* Adding the wikitables parser

* Fix dockerfile and docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.