Skip to content

Add metric structure - #109

Open
NicolasCorvol wants to merge 5 commits into
mainfrom
issue_107_add_generic_metric
Open

Add metric structure#109
NicolasCorvol wants to merge 5 commits into
mainfrom
issue_107_add_generic_metric

Conversation

@NicolasCorvol

Copy link
Copy Markdown
Contributor

As described in issue #107, the package lacks a way to handle metrics which would be beneficial to users who want to compare the performances of several policies on their benchmark.

What this adds
The main file added is src/Utils/metric.jl

The main method is the AbstractMetric{B<:AbstractBenchmark}: every metric is bound to a benchmark instance
A nice sub structure is also define : Metric{B,F} => takes as input a callable metric wrapping any (bench, dataset) -> Real for quick user-defined metrics without writing a dedicated type.
It also implements several evaluate_metric that can handle a single dataset, multiple datasets (returns the vector of values) or multiple pairs ("policy", datasets), which returns a Vector{MetricStats} for one-call multi-policy evaluation.
MetricStats : per-dataset values with mean_metric/std_metric/quantile_metric functions. It carries a label naming the policy that produced it (e.g. a Policy's name).

We also implement two predefined metrics: reward and gap.
RewardMetric(bench; op=mean) - works for every benchmark:

  • mean objective_value for static
  • SAA-wrapped for stochastic benchmarks
  • aggregated sample.extra.reward for dynamic

RelativeGapMetric(bench, statistical_model, maximizer; op=mean) - wraps the existing compute_gap as a metric

It also adds plotting functions : boxplots for metric comparisons

Notes / follow-ups

  • A clean evaluate_policies function could be nice to have to be able to evaluate several stochastic benchmarks on multiple policies.
  • Linking this PR to the one refactoring the policies could be very nice too

@NicolasCorvol
NicolasCorvol requested a review from BatyLeo July 22, 2026 17:16
@NicolasCorvol NicolasCorvol self-assigned this Jul 22, 2026
@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.95181% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/Utils/metric.jl 50.00% 6 Missing ⚠️
src/Utils/interface/dynamic_benchmark.jl 87.87% 4 Missing ⚠️
Files with missing lines Coverage Δ
ext/DFLBenchmarksStatsPlotsExt.jl 100.00% <100.00%> (ø)
ext/plots/metric_plots.jl 100.00% <100.00%> (ø)
src/DecisionFocusedLearningBenchmarks.jl 100.00% <ø> (ø)
src/Utils/Utils.jl 100.00% <ø> (ø)
src/Utils/interface/static_benchmark.jl 100.00% <100.00%> (ø)
src/Utils/interface/dynamic_benchmark.jl 92.00% <87.87%> (-8.00%) ⬇️
src/Utils/metric.jl 50.00% <50.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@BatyLeo BatyLeo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the PR, that's a good first draft. I put some first comments, I think several things can be improved, but we need to first converge on the Policy PRs in order to see what's the best way to manage metrics.

Comment thread ext/DFLBenchmarksStatsPlotsExt.jl
"""
$TYPEDEF

Abstract type for evaluation metrics, used to evaluate and compare [`Policy`](@ref)s

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: we probably can compare any AbstractPolicy, not only those of type Policy. This comment can be updated once the other PR on policies is merged

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed !

on a benchmark. Parametrized by the benchmark type `B` the metric is bound to,
this makes sense since an evaluation process is always benchmark-specific.

The metrics logic is splitted between static and dynamic benchmark:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: what about stochastic benchmarks, are they considered as static in this case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed this feels weird... can we improve the naming ? Should we consider 3 metric types...?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a comment on this in commit 10617f7.
indeed, for now, the package handles the evaluation of stochastic benchmarks via SampleAverageApproximation which is an AbstractStaticBenchmark ==> we handle their metric through AbstractStaticMetric.
I would be glad to help with a native implementation for evaluating stochastic benchmark (with a one step rollout ?)

Comment thread src/Utils/metric.jl Outdated
Comment thread src/Utils/metric.jl Outdated
Comment thread src/Utils/metric.jl Outdated
Comment thread src/Utils/metric.jl Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants