Skip to content

[codex] Fix SSIM checkpoint selection and shared metric direction - #183

Merged
jbohnslav merged 8 commits into
masterfrom
test/metric-direction-regressions
Sep 16, 2026
Merged

jbohnslav merged 8 commits into
masterfrom
test/metric-direction-regressions

Conversation

@jbohnslav

@jbohnslav jbohnslav commented Sep 16, 2026 •

Copy link
Copy Markdown
Owner

Fixes #182.

Flow training records SSIM as a loss, but checkpoint selection inferred direction from whether the metric name contained loss. It therefore saved the highest SSIM loss as the best checkpoint, which downstream feature-extractor training preferentially loads. This change minimizes SSIM while preserving F1 maximization.

Metrics now carry a validated key_metric_mode, shared by checkpoints, learning-rate scheduling, and the existing early-stopping is_error flag. The same direction resolver drives Ray ASHA, HyperOpt, and trial selection, and best-epoch confusion plotting. New HDF5 files persist direction; legacy files fall back to known metric names. Custom objectives must supply an explicit min/max override. The public classification get_metrics() factory accepts and forwards key_metric_mode, with min/max and default-direction regression coverage.

The audit also found that early stopping minimized classification F1 and could crash on its first non-improving score. Both are fixed. EmptyMetrics initialization is corrected to preserve loss semantics during automatic tuning.

Validation:

  • Before fixes: 17 failing regression cases and 16 passing controls.
  • After fixes and expanded coverage: QT_QPA_PLATFORM=offscreen uv run pytest -q --tb=short → 85 passed, 5 GPU tests deselected.
  • Ruff checks and git diff --check pass.
  • Tests cover checkpoint comparison, scheduler behavior, early stopping, both tuning entrypoints and search methods, custom direction overrides, and legacy/new metric files. Ray boundaries are mocked; no live Ray or GPU job was run.

Based directly on master; independent of the Torch/Lightning upgrade in #180. This corrects future checkpoint selection but does not recover previously discarded epochs.

Claude review workflow diagnostics:

Release and review cadence:

  • Bump package and lock metadata to 0.5.0; uv lock --check and uv build pass, with wheel/sdist metadata verified. Merging into master triggers the existing GitHub Release/PyPI publishing workflow.
  • Claude review runs when a non-draft PR opens or a draft becomes ready, rather than on synchronize/reopened events.
  • Remove tracked .kd files while preserving the existing ignore rule and local ticket data.

@claude

claude Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Claude finished @jbohnslav's task in 2m 39s —— View job


Claude Code Review

  • Gather context (PR diff, description, existing Codex review comment)
  • Review deepethogram/metrics.py, base.py, callbacks.py, stoppers.py, viz.py, tune/*.py, and tests
  • Post findings

This PR correctly fixes the SSIM checkpoint-selection bug and centralizes metric direction (key_metric_mode) across checkpointing, LR scheduling, early stopping, and confusion-matrix plotting. The regression tests in tests/test_metric_directions.py are thorough (checkpoint comparator, scheduler, early-stopping edge case, tuning entrypoints, legacy/new HDF5 files, custom overrides).

Findings

1. get_metrics() factories don't expose key_metric_mode (agrees with Codex's inline comment on metrics.py:684)

  • deepethogram/feature_extractor/train.py:567 (get_metrics) and deepethogram/flow_generator/train.py:350 (get_metrics) construct Classification/OpticalFlow without a key_metric_mode parameter.

  • Classification.compute() (metrics.py:914-916) auto-records any data key containing "loss" as a metric (for key in data.keys(): if "loss" in key and key != "loss": metrics[key] = np.mean(...)), so a hypothetical custom key_metric like "auxiliary_loss" would hit the new get_metric_mode ValueError in Metrics.__init__, with no parameter on the public factory to supply the override.

  • Not currently exercised (today's call sites hardcode "f1_class_mean_nobg", "f1_class_mean", and "SSIM", all of which resolve correctly), but it's a real gap if key_metric is ever made configurable for classification/flow training the way it already is for tune.yaml. Worth threading key_metric_mode through both get_metrics signatures for consistency with the tuning entrypoints, which already accept it via cfg.tune.key_metric_mode.

    Fix this →

Verified as correct

  • EmptyMetrics.__init__ positional-arg fix (metrics.py:838) now matches Metrics.__init__'s signature (run_dir, key_metric, name, num_parameters), preserving "loss"/"min" semantics.
  • StopperCallback (callbacks.py:227) sets is_error dynamically from key_metric_mode on every validation epoch, so the hardcoded is_error=True default in stoppers.get_stopper (used only for standalone/legacy callers) is always overridden before use in the actual Lightning training path — the comment there is accurate.
  • viz.plot_confusion_from_logger correctly falls back to inferring direction from known metric names via get_metric_mode for legacy HDF5 files lacking key_metric_mode.
  • get_metric_mode's known min/max sets cover all key metrics actually used by current call sites (SSIM, loss, f1_class_mean(_nobg)).

No other correctness issues found in the diff. The .github/workflows/claude-code-review.yml changes (job-scoped token, sanitized failure reporting) look reasonable and are outside this agent's ability to modify/test further.

@jbohnslav
jbohnslav marked this pull request as ready for review September 16, 2026 13:58
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-16T14:01:25.550885Z 50e9900 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 50e9900920

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread deepethogram/metrics.py Resolved
@jbohnslav
jbohnslav merged commit 2a7b081 into master Sep 16, 2026
5 checks passed
@jbohnslav
jbohnslav deleted the test/metric-direction-regressions branch September 16, 2026 14:23
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.

Flow generator saves its WORST epoch as the 'best' checkpoint (key_metric='SSIM' with direction inferred from the name)

1 participant