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

Various fixes related to the variable free wikitables world #1917

Merged
merged 17 commits into from Oct 23, 2018

Conversation

@pdasigi
Copy link
Contributor

commented Oct 17, 2018

@matt-gardner This is now ready to review. Other than adding a dataset reader, and (mostly) copying the original MML model to make a variable free variant, the changes are:

  1. Moved some predicates in the variable free world into local mappings to make them table-specific (for example, if the table has no date columns, productions leading to functions like filter_date_greater are not valid).
  2. Related to the above change, when substituting a MultiMatchNamedBasicType within a ComplexType to get productions that lead to it, only those that are in the table are allowed (For example, productions like s -> [<r,<g,s>> r, f] are not allowed if the table does not have number columns (f)).
  3. Added a method within TableQuestionContext to return a KnowledgeGraph.

@pdasigi pdasigi requested a review from matt-gardner Oct 19, 2018

# (batch_size, num_entities, num_neighbors) or None
neighbor_indices = self._get_neighbor_indices(world, num_entities, encoded_table)

if neighbor_indices is not None:

This comment has been minimized.

Copy link
@pdasigi

pdasigi Oct 19, 2018

Author Contributor

This part is new. It's possible that none of the instances in a given batch have entities with neighbors, because of the way the knowledge graph is defined.

@pdasigi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

@matt-gardner Based on our offline conversation about separating research code into a separate repo, I'm removed several changes from this PR to the other repo. I kept all the changes I made to the files already in the master branch in this repo. So, the ones I took out are the new dataset reader and model code, and their tests and fixtures. The rest of the summary above still holds true.

@pdasigi pdasigi changed the title Variable free dataset reader and MML model for WikiTables Various fixes related to the variable free wikitables world Oct 23, 2018

@matt-gardner
Copy link
Member

left a comment

I'd probably recommend just copying the files from here that you want to modify over to the other repo, for next time. We can copy the changes back once things are stable.

# specified. We want to do this only when the table has a date column. This is because
# the knowledge graph is also constructed in such a way that -1 is an entity with date
# columns as the neighbors only if any date columns exist in the table.
self._map_name(f"num:-1", keep_mapping=True)

This comment has been minimized.

Copy link
@matt-gardner

matt-gardner Oct 23, 2018

Member

I think we may want to handle the -1 in dates separately from the typical "number" entity mechanism here - maybe have it be a global action, not a linked action at all. But we can worry about that later.

@pdasigi pdasigi force-pushed the pdasigi:wikitables_var_free_model branch from 78b64dd to c430f4f Oct 23, 2018

@pdasigi pdasigi merged commit c6fb86d into allenai:master Oct 23, 2018

3 checks passed

Pull Requests (AllenNLP Library) TeamCity build finished
Details
codecov/patch 93% of diff hit (target 90%)
Details
codecov/project 92% (+<1%) compared to aeb2fc3
Details

@pdasigi pdasigi deleted the pdasigi:wikitables_var_free_model branch Oct 23, 2018

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.