-
Notifications
You must be signed in to change notification settings - Fork 45
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
Evaluation results class for easier access to results #1326
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: elronbandel <[email protected]>
@@ -320,6 +320,30 @@ def prepare(self): | |||
) | |||
|
|||
|
|||
class EvaluationResults(list): | |||
@property | |||
def score(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "score" is a problematic name (should have been "scores"). Do we want to start change it here. We could "global_scores", "group_scores" and "subset_scores", "instance_scores" attributes. Slowly we will move off the list and use only the attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most useful should be the shortest most natural name. So assuming it is used like this: results.score the most natural way is to call it something like results.summary
The slow movement from list can start by using to_list() for most cases and examples that actually access the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is worthwhile to discuss the names, as they stay with us for a long time.
"result.score" sounds like it returns a single number. Note the result also contains instances which are not only score.
result.scores -
result.subset_scores
result.group_scores
result.instances - (prediction, references, task data, and scores)
@property | ||
def groups(self): | ||
if "groups" not in self[0]["score"]: | ||
raise ValueError("Groups scores not found try using group_by in the recipe") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not clear. We should have a UnitxtError("No groups were defined using 'group_by' in the recipe. For more information ....) and point the
documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
@property | ||
def subsets(self): | ||
if "subsets" not in self[0]["score"]: | ||
raise ValueError("Subsets scores not found try using Benchmark") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a UnitxtError("No subsets were defined using using the Benchmark in the recipe. For more information ....) and point the
documentation
import pandas as pd | ||
|
||
# Flatten and load into DataFrame | ||
return pd.json_normalize(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the output look like? Maybe since it's a one line, we don't need it. As it might make sense to convert different scores (global, subsets) seperately,
I think that as part of this change (once it's finalized) we should change all the example and relevant documentation to use it - so people will get used to the new way of working. |
No description provided.