restructure aggregation of InstanceMetric , enabling to expand grouping and filtering option on to GlobalMetric #752

dafnapension · 2024-04-04T21:09:48Z

No description provided.

codecov · 2024-04-04T21:27:08Z

Codecov Report

Attention: Patch coverage is 96.17834% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 91.98%. Comparing base (5783d76) to head (22bbb8e).
Report is 43 commits behind head on main.

❗ Current head 22bbb8e differs from pull request most recent head eb7f439. Consider uploading reports for the commit eb7f439 to get more accurate results

Files	Patch %	Lines
src/unitxt/metrics.py	95.34%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #752      +/-   ##
==========================================
+ Coverage   91.48%   91.98%   +0.50%     
==========================================
  Files         100      103       +3     
  Lines       10169    10668     +499     
==========================================
+ Hits         9303     9813     +510     
+ Misses        866      855      -11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yoavkatz · 2024-04-08T08:02:08Z

@elronbandel , @dafnapension

There are currently additional ways to group metrics (note I'm not sure if they are the best way either), but my general message is we see how add functionality without expanding the APIs of metrics more and more.

We should have one recommended way to group, and if we implement a new one, and replace other uses.

The proposed method here allows arbitrary conditions, buts requires listing them specifically. On the other hand, grouping by field value as done today is more natural is some cases, where the options are not known.

Another suggestion is to share the proposed specification before the PR, so feedback can be given early.

dafnapension · 2024-04-09T15:03:02Z

Hi @yoavkatz , I hope the current suggestion for a class that takes care of splitting/grouping is more in the direction you had in mind.
I think that if accepted, this class could also play a role in GlobalMetric (as it does in the current PR), and perhaps will unify all the different grouping that are currently tailored to Accuracy (and only to it).
Another issue that I think we may want to address is the name of the score stored in ["score"]["global"] for this groupped version of the evaluation.
I vote for generating children to ["score"]["global"], each child named by the group it represents (as suggested in this PR) and inside this child, things will look exactly as they look now (no splitting / grouping whats so ever) in ["global"]

yoavkatz · 2024-04-09T15:19:48Z

src/unitxt/metrics.py

+# InstanceMetric's  compute_instance_scores(stream)
+
+
+class Aggregator(Artifact):


Is this related to this PR?

A start of an attempt to unify the aggregators over the InstanceMetrics

yoavkatz · 2024-04-09T15:26:35Z

Hi @yoavkatz , I hope the current suggestion for a class that takes care of splitting/grouping is more in the direction you had in mind. I think that if accepted, this class could also play a role in GlobalMetric (as it does in the current PR), and perhaps will unify all the different grouping that are currently tailored to Accuracy (and only to it). Another issue that I think we may want to address is the name of the score stored in ["score"]["global"] for this groupped version of the evaluation. I vote for generating children to ["score"]["global"], each child named by the group it represents (as suggested in this PR) and inside this child, things will look exactly as they look now (no splitting / grouping whats so ever) in ["global"]

Yes. I think better. Elron said there is a grander design, which we should review.

dafnapension · 2024-05-02T05:24:04Z

Hi @elronbandel and @yoavkatz,
I suggest here a simplifying restructure of the different variations of instance-metric-aggregation (grouping - and then which ci, filtering, control-comparison) such that: (1) they are independent, and hence filtering and control-comparison are now applicable to the whole stream, and not only when grouping, and (2) easily applicable to GlobalMetric as well, as demonstrated in the tests I added, since the 'engine' of these variations now sits in MetricWithConfidenceInterval, and to BulkInstanceMetric too.
Please see if this makes sense.
MetricWithConfidenceInterval thus becomes the manager of all the work-along-the-instances for metric evaluation (as opposed to within each instance): filtering - grouping - aggregating over individual instance scores - and computing of CI.