Remove target_for_fewshot_sorting and duplicate target calls
#241
Conversation
|
Hi ! Thanks for your PR, it appears the tests are not passing, I already took a look and it looks good but I will be reviewing more thourouly once the tests pass :) |
sadra-barikbin
force-pushed
the
deciding-target-for-fewshot-sorting
branch
from
df4d57b to
dc81343
Compare
| choices=[" " + i for i in LETTER_INDICES[: len(line["endings"])]] + ([""] if line["__few_shot"] else []), | ||
| gold_index=gold_ix, # -1 for test, | ||
| instruction="The following are multiple choice questions (with answers) about common sense.\n\n", | ||
| target_for_fewshot_sorting=line["endings"][gold_ix] if gold_ix > -1 else "", | ||
| specific={ |
There was a problem hiding this comment.
Are you sure this conversion is correct?
There was a problem hiding this comment.
I changed it to a safer alternative.
| gold_index=gold_ix, | ||
| instruction=f"The following are multiple choice questions (with answers) about {subject.replace('_', ' ')}.\n\n", | ||
| target_for_fewshot_sorting=line["choices"][gold_ix], # specific to HELM evals |
There was a problem hiding this comment.
The conversion is incorrect here.
Helm evals are actually doing a super weird thing where the actual choice is used for the few shot, and the key for the evaluation. (I think we should change it anyway since it makes no sense, so you'll just need to remove the comment about helm)
There was a problem hiding this comment.
Is it correct now? choice contents for fewshot and labels(A, B, C, D) for eval.
There was a problem hiding this comment.
Now I set target_for_fewshot_sorting to the label (A,B,C,D). This way fewshot sampling becomes balanced on labels. May I keep this or revert it to be the choice content?
| return Doc( | ||
| task_name=task_name, | ||
| query=query, | ||
| choices=["A", "B"], | ||
| gold_index=gold_ix, | ||
| instruction="The following are multiple choice questions (with answers) about common sense.\n", | ||
| target_for_fewshot_sorting=[line["sol1"], line["sol2"]][gold_ix], |
There was a problem hiding this comment.
It's the same here, you changed the logic
There was a problem hiding this comment.
I changed this accordingly.
|
Hi Sadra, |
clefourrier
changed the title
target_for_fewshot_sorting for another purposetarget_for_fewshot_sorting and duplicate target calls
|
Hello @clefourrier , but this helps in fewshot sampling to become balanced. When For example, we have a function calling task. Our golds are json strings e.g. |
|
I see what you need it for! Hm let me think about it - I still think we should remove it for the HELM and keep only one version of a number of the tasks we have, but I understand your use case! |
|
@sadra-barikbin I added the way I would code it in #393 - (I can't push to your PR branch unless I'm editing from the web interface). Can you bring the modifications (prompt_manager and requests) over to your PR? |
|
Edit: will close this one and finish the rest in the other PR - can you check you got all you're needing over there? |
Removedtaskarg fromFewShotSampler.init_fewshot_sampling_balanced()because it no longer needs it.fewshotarg fromtask.doc_to_target()anddoc.get_golds()as it's no longer used.get_target_for_fewshot_sorting()toDocwhich givestarget_for_fewshot_sortingif it's set, otherwise gives the gold.Closes #240