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

Aggregation Prediction uses select column from the ground truth #12

Open
ghost opened this issue Jan 17, 2018 · 4 comments
Open

Aggregation Prediction uses select column from the ground truth #12

ghost opened this issue Jan 17, 2018 · 4 comments

Comments

@ghost
Copy link

ghost commented Jan 17, 2018

The below code snippet looks like using the column position for select in evaluation as well as training.

gt_sel_seq = [x[1] for x in ans_seq]

col_name_len, col_len, col_num, gt_sel=gt_sel)

chosen_sel_idx = torch.LongTensor(gt_sel)

Don't you think the model should predict the column for select instead of the column given as ground truth before aggregation prediction?
In fact, the select column will not be given in prediction time when applying this to real worlds.

You made selection prediction, so the output could be fed with the aggregation prediction.
In result, the evaluation number might be wrong while comparing with the original paper, seq2sql.

@guotong1988
Copy link

#3

@ghost
Copy link
Author

ghost commented Jan 26, 2018

@guotong1988

Here is the code snippet to replace the ground truth to the prediction.
So it will work well in evaluation as well as training.

        if gt_sel is None:
            if self.trainable_emb:
                x_emb_var, x_len = self.sel_embed_layer.gen_x_batch(q, col)
                col_inp_var, col_name_len, col_len = \
                        self.sel_embed_layer.gen_col_batch(col)
                max_x_len = max(x_len)
                sel_score = self.sel_pred(x_emb_var, x_len, col_inp_var,
                        col_name_len, col_len, col_num)
            else:
                sel_score = self.sel_pred(x_emb_var, x_len, col_inp_var,
                        col_name_len, col_len, col_num)

            pred_sel = []
            for b in range(B):
                pred_sel.append(np.asscalar(np.argmax(sel_score[b].data.cpu().numpy())))

            # use the preds for aggregator
            gt_sel = pred_sel

@ThomasLecat
Copy link

ThomasLecat commented Feb 5, 2018

@deepcoord

Thanks for the code snippet.
I have a question though:

If I'm not mistaken, with your fix, sel_score is computed twice:

  • a first time by your piece of code before predicting the aggregator
  • a second time by the original code after predicting the aggregator

Wouldn't it be simpler to just reverse the order in which sel_score and agg_score are computed in the forward method in sqlnet.py ? By doing so, we could compute sel_score only one time and add a shorter snippet of code just before predicting the aggregator :

if gt_sel is None:
      sel_score_np = sel_score.data.cpu().numpy()
      gt_sel = [np.asscalar(np.argmax(sel_score_np[b])) for b in range(B)]

Am I missing something?

@xiaojunxu
Copy link
Owner

Thanks for recognizing the issue! Yes, this will lead to a wrong break-down result on aggregator but will not affect the overall acc_qm or acc_ex. I will fix the bug soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants