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

Evaluation results class for easier access to results #1326

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion src/unitxt/metric_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,30 @@ def prepare(self):
)


class EvaluationResults(list):
@property
def score(self):
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@yoavkatz yoavkatz Nov 4, 2024

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)

return self[0]["score"]["global"]

@property
def groups(self):
if "groups" not in self[0]["score"]:
raise ValueError("Groups scores not found try using group_by in the recipe")
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

return self[0]["score"]["groups"]

@property
def subsets(self):
if "subsets" not in self[0]["score"]:
raise ValueError("Subsets scores not found try using Benchmark")
Copy link
Member

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

return self[0]["score"]["subsets"]

def to_df(self):
import pandas as pd

# Flatten and load into DataFrame
return pd.json_normalize(self)
Copy link
Member

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,



def _compute(
predictions: List[str],
references: Iterable,
Expand All @@ -340,7 +364,7 @@ def _compute(
multi_stream = operator(multi_stream)

stream = multi_stream[split_name]
return list(stream)
return EvaluationResults(stream)


"""
Expand Down
Loading