Remove pickle from data formats, memmap for Tabular predictions #45
Conversation
| class EnsembleScorer: | ||
| def __init__(self, | ||
| zeroshot_pp: TabularModelPredictions | dict, | ||
| zeroshot_gt: Dict[str, Dict[int, Dict[str, Any]]], | ||
| tid_to_dataset_dict: dict, |
There was a problem hiding this comment.
tid_to_dataset_dict should not be passed here. Instead, dataset should always be passed to evaluate_task as was done before. self.zeroshot_gt and self.zeroshot_pp should not use different keys to fetch the required information.
There was a problem hiding this comment.
Previously EnsembleScorer had no concept of tid, which was the main point of the class, to remove the need to worry about tid and dataset conversions.
There was a problem hiding this comment.
For most of this, I can handle, but please align self.zeroshot_gt and self.zeroshot_pp to use the same key (probably dataset is best)
There was a problem hiding this comment.
Instead, dataset should always be passed to evaluate_task as was done before. self.zeroshot_gt and self.zeroshot_pp should not use different keys to fetch the required information.
I did not change how evaluate_task is called, I just renamed dataset to tid as the field that was passed was effectively tid.
Previously zeroshot_pp had the dictionary internally (with a method rename_datasets) so the mapping is not added, it is just removed from zeroshot_pp.
I also think it would be good to use tid: int or dataset: str consistently when getting ground-truth, predictions etc but it is quite involved and I think it is probably out of scope for this PR.
There was a problem hiding this comment.
Previously EnsembleScorer had no concept of tid, which was the main point of the class, to remove the need to worry about tid and dataset conversions.
Reading this one, it seems that you mean that the name dataset should be kept, that one can be done but we have to reintroduce rename_dict or update the groundtruth class, I will take a look if it is possible without too much change.
There was a problem hiding this comment.
I took a look and it is not straight-forward to unify dataset/tids in zeroshot_gt and zeroshot_pp. Of-course just unifying those two classes is trivial but the difficulty is with integrating all classes with a new naming. This is because right now, data is accessed in many different ways in different part of the code and classes (for instance tasks are addressed by a string f"{tid}_{fold}" by a float 123.4 where 123 is the task and 4 is the fold or by a tuple, by dataset or by tid).
This PR is already covering a lot by revamping all the pickles data structures and associated classes and updating all context files and I think it should be the content for another PR (which would for instance always address data with tid and fold by a tuple which would make the change much easier but would have to touch all the classes).
There was a problem hiding this comment.
Sure, I can try to see if it can be unified in a separate PR.
| if rename_dict_inv: | ||
| dataset = rename_dict_inv.get(dataset, dataset) | ||
| return str(Path(output_dir) / f'{dataset}' / f'{fold}.pkl') | ||
| class TabularPredictionsMemmap(TabularModelPredictions): |
There was a problem hiding this comment.
Please include a conversion method from TabularPredictionsMemmap to TabularPredictionsInMemory. It is easy to convert TabularPredictionsInMemory to TabularPredictionsMemmap, but the same is not currently true for the reverse.
There was a problem hiding this comment.
Sure, I have added one.
WIP, the evaluation works from scratch and takes much less memory but I am still checking a few things.
A few notes: