Fix make metric aggregation use explicit logging semantics (Closes #343) - #344
Conversation
|
@sadamov @joeloskarsson Friendly ping to review it when u have time :D |
c5607c3 to
3991200
Compare
3991200 to
7ded2f1
Compare
There was a problem hiding this comment.
Great work🤩, the metric-as-object design is a clear improvement over the per-name spec approach. A few issues from a careful read:
Critical: output_std is initialized but never populated in test_step will crash aggregate_and_plot_metrics when self.output_std=True.
Medium: The fallback else branch in aggregate_and_plot_metrics still applies the broken universal-linear-rescaling rule for any unregistered metric. Re-introduces the exact bug #343 fixes, just behind a gate. Consider removing it.
Low: WMSE and WMAE class docstrings still describe linear rescaling they should mention they're dimensionless and not rescaled.
Tests look thorough for the registered metrics adding one end-to-end run through test_step with output_std=True would have caught #1.
If you could rebase it on the current main , it would be really helpful in reviewing !
| # Compute the built-in evaluation metrics for error maps. test_metrics | ||
| # may also contain subclass-specific entries that are populated | ||
| # differently and only aggregated later. | ||
| for metric_name in ("mse", "mae"): |
There was a problem hiding this comment.
This loop only populates test_metrics["mse"] and test_metrics["mae"], but test_metrics["output_std"] is still initialized as an empty list at line 148 when self.output_std=True. The old manual mean_pred_std block that populated it was removed, but OutputStd() is never called anywhere to fill it.
When self.output_std=True, on_test_epoch_end will hit torch.cat([]) in aggregate_and_plot_metrics and crash with RuntimeError: torch.cat(): expected a non-empty list of Tensors.
The PR description says output_std was "made a real metric object with the same callable interface as the others" but the call site was never wired up. I suggest extending the loop:
metric_names = ["mse", "mae"]
if self.output_std:
metric_names.append("output_std")
for metric_name in metric_names:
...| metric_tensor, self.state_std | ||
| ) | ||
| display_name = metric_obj.display_name | ||
| else: |
There was a problem hiding this comment.
This else branch re-implements the exact universal-linear-rescaling rule that issue #343 calls out as incorrect just gated behind "is the metric unregistered?". Any future custom metric added to a subclass's test_metrics/val_metrics without registering in DEFINED_METRICS will silently hit the original bug again.
Given the goal of the PR is to eliminate the implicit universal-rescaling assumption, I'd argue this fallback shouldn't exist. Two cleaner options:
- Require registration: raise a clear error for unknown metric names, forcing any new metric to go through the
BaseMetricinterface. This is what the new tests already check for registered names extend the contract. - Default behavior on
BaseMetric: if every metric must be aBaseMetric, the fallback is unreachable by construction.
If the fallback must stay for some reason (e.g. you know of an out-of-tree subclass that depends on it), please leave a clear comment naming what it's for
| """ | ||
| Weighted Mean Squared Error (weighted by 1/pred_std^2). | ||
| Logged as WRMSE (sqrt applied after averaging, then linear rescale). | ||
| """ |
There was a problem hiding this comment.
The class docstring says "Logged as WRMSE (sqrt applied after averaging, then linear rescale)", but WMSE.rescale below returns the tensor unchanged (no linear rescale because WMSE is dimensionless). Looks like a copy-paste leftover from MSE.
| """ | |
| """ | |
| Weighted Mean Squared Error (weighted by 1/pred_std^2). | |
| Logged as WRMSE (sqrt applied after averaging). Not rescaled weighted | |
| metrics are dimensionless in normalized space. | |
| """ |
Co-authored-by: Jeevant Prakhar Singh <anupamasinghsrinet1976@gmail.com>
|
Hi @kshirajahere, thanks for this and sorry for the long wait! Two things have shifted since you opened this that make a fresh rebase worth doing before review:
The bug it fixes (universal linear rescaling assumption in |
3 participants
Describe your changes
This PR fixes metric aggregation/logging in
ARModel.aggregate_and_plot_metrics()by replacing the current universal linear rescaling rule with explicit per-metric logging semantics.Summary of changes
MetricLoggingSpecinneural_lam.metricsARModel.aggregate_and_plot_metrics()to delegate to metric-specific logging behavior instead of assuming all metrics should be linearly rescaled bystate_stdmse -> rmsewith sqrt + linear rescalingmaewith linear rescalingoutput_stdwith linear rescalingnllwith no linear rescalingMotivation / context
aggregate_and_plot_metrics()previously assumed that every aggregated metric could be converted from standardized space to logged units via the same linear rescaling rule.That assumption is only valid for some metrics. As evaluation support expands, especially for probabilistic metrics, logging behavior needs to be explicit per metric rather than hidden in a universal post-processing rule.
Dependencies
No new external dependencies.
Issue Link
Closes #343
Type of change
Checklist before requesting a review
pullwith--rebaseoption if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee