Closed DeNeutoy closed 6 years ago
Thanks for catching this! I'll definitely work on a fix and follow up (but it might take a few days).
On adding it to allennlp, is there a place I should look for guidance on structuring a contribution? (coding style, etc)
Oh, and one quick note - the evaluation for the non-baseline systems was totally independent of this, so those numbers should not be affected by this bug.
Great! should be easy to fix, this snippet should do the trick:
if args.split is not None:
# No mutation of `dataset` fixes things
if str(args.split) == str(dataset):
dataset_to_use = "test"
else:
dataset_to_use = "train"
else:
dataset_to_use = dataset_split
...
yield (dataset_to_use, insert_variables(sql, sql_vars, text, text_vars))
R.e contributing to allennlp - I will send you details once I have merged in some utility functions which will help with reading the data.
Thanks, I ended up going with a simpler fix (see PR #11) and it looks like there were no substantial changes in results.
I believe there is a test set leakage in the case that the dataset requires the use of cross validation ( Academic, IMDB, Restaurant and Yelp datasets) and the
args.query_split
flag is set toTrue
.This bug affects the baseline script due to this function copied verbatim from https://github.com/jkkummerfeld/text2sql-data/blob/master/systems/baseline-template/text2sql-template-baseline.py#L114 with comments added to demonstrate the problem:
Note that this bug doesn't affect the results for the question split, because it is guarded against here, meaning the
dataset
variable is reset correctly each time.As an example, this following JSON blob (with --query-split=True and --split=1) will get split across the test(first sentence) and training set(second two sentences), when it should only be in the test set. This is a particularly tricky problem, because some of the sentences which have the same SQL query are actually identical, meaning that the test split essentially contains duplicates from the training split.
Fortunately, I think this bug actually reinforces the point of the paper - the validation scores are only going to fall for the datasets for which this happens for, which are as you note in section 5.4 of your paper:
"For the three datasets developed by the databases community, the effect of question-querysplit is far less pronounced."
Possibly, this bug is an underlying cause of this interpretation, given that these 3 datasets are all ones which use cross-validation.
This is a great resource btw - currently we are working on adding it to allennlp to do some type constrained decoding based off of a SQL grammar. If you are interested, we'd be happy to accept PRs to get your baseline into allennlp, where it would be tested and reviewed etc.