feat(metrics): expose alpha_min + alpha_max kwargs in microssim#41
Merged
Conversation
Adds alpha_max to get_ri_factor / get_global_ri_factor / MicroSSIM.__init__ so callers can lift the bracket cap for pathological fits without monkey- patching internals. Bumps the default from 1e3 to 1e6 — the old cap could spuriously fail on heavily down-scaled predictions (optimum > 1e3) before the fit even surfaced; new default safely covers those without changing the result for well-conditioned inputs. Also adds regression tests pinning the MicroMS3IM(ri_factor=...) + offset/max_val constructor path so callers can load a per-(model, organelle) RI factor calibrated once and reuse it across all evaluation calls (no re-fit needed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…xport) Pre-PR cleanup from the review pass: - Re-export ALPHA_MAX_DEFAULT from cubic.metrics.microssim so callers can reference the default without reaching into the ri_factor submodule. - Validate alpha_max eagerly in MicroSSIM.__init__ (NaN, ±inf, <=1 now raise at construction instead of after an expensive fit-time pass). - Drop a stale "[1e-3, 1e3]" reference from a test docstring after the default cap bump. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Reviewer's GuideIntroduces a configurable alpha_max cap for RI-factor bracketing in MicroSSIM, raises the default cap from 1e3 to 1e6, threads the parameter through the RI-factor helpers and MicroSSIM, and adds tests and exports to support external calibration and pinned RI factors. Sequence diagram for configurable alpha_max propagation in MicroSSIM fitsequenceDiagram
actor User
participant MicroSSIM
participant get_global_ri_factor
participant get_ri_factor
participant _bracket_root
User->>MicroSSIM: __init__(bg_percentile, offset_gt, offset_pred, max_val, ri_factor, alpha_max)
MicroSSIM->>MicroSSIM: validate alpha_max
MicroSSIM->>MicroSSIM: store _alpha_max
User->>MicroSSIM: fit(gt, pred)
MicroSSIM->>get_global_ri_factor: get_global_ri_factor(gt_norm, pred_norm, alpha_max=_alpha_max)
get_global_ri_factor->>get_ri_factor: get_ri_factor(pooled, alpha_max)
get_ri_factor->>get_ri_factor: validate alpha_max
get_ri_factor->>_bracket_root: _bracket_root(flat, f1, alpha_max)
_bracket_root-->>get_ri_factor: lo, hi, f_lo, f_hi
get_ri_factor-->>get_global_ri_factor: alpha
get_global_ri_factor-->>MicroSSIM: alpha
MicroSSIM->>MicroSSIM: set _ri_factor = alpha
MicroSSIM-->>User: MicroSSIM (fitted)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
alpha_maxvalidation logic is duplicated in bothget_ri_factorandMicroSSIM.__init__; consider factoring this into a small shared helper to keep behavior and error messages in sync. - Now that
MicroSSIMexposes analpha_maxknob, you may want to surface a corresponding parameter onMicroMS3IM(or document how to tune it indirectly) so callers using the higher-level API can control the same behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `alpha_max` validation logic is duplicated in both `get_ri_factor` and `MicroSSIM.__init__`; consider factoring this into a small shared helper to keep behavior and error messages in sync.
- Now that `MicroSSIM` exposes an `alpha_max` knob, you may want to surface a corresponding parameter on `MicroMS3IM` (or document how to tune it indirectly) so callers using the higher-level API can control the same behavior.
## Individual Comments
### Comment 1
<location path="cubic/metrics/microssim/micro_ssim.py" line_range="65-70" />
<code_context>
offset_pred: float | None = None,
max_val: float | None = None,
ri_factor: float | None = None,
+ alpha_max: float = ALPHA_MAX_DEFAULT,
) -> None:
+ if not (np.isfinite(alpha_max) and alpha_max > 1.0):
+ raise ValueError(f"alpha_max must be a finite float > 1; got {alpha_max}")
self._bg_percentile = bg_percentile
</code_context>
<issue_to_address>
**suggestion:** alpha_max is validated even when ri_factor is supplied and alpha_max is documented as having no effect in that case
Because the constructor still validates `alpha_max` even when `ri_factor` is provided, callers may see a `ValueError` for a parameter that is documented as having no effect in this case. To avoid this mismatch, either skip `alpha_max` validation when `ri_factor` is not `None`, or update the docs to state that `alpha_max` is always validated regardless of whether it affects the computation.
```suggestion
ri_factor: float | None = None,
alpha_max: float = ALPHA_MAX_DEFAULT,
) -> None:
# Only validate alpha_max when it is actually used (i.e., when ri_factor is not supplied)
if ri_factor is None and not (np.isfinite(alpha_max) and alpha_max > 1.0):
raise ValueError(f"alpha_max must be a finite float > 1; got {alpha_max}")
self._bg_percentile = bg_percentile
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR makes the MicroSSIM “range-invariant factor” (RI-factor) fitting more configurable and robust by exposing an alpha_max cap through the public API, increasing the default cap to handle heavily down-scaled predictions, and adding tests that document/lock in these behaviors (including scoring with a pinned, externally-calibrated ri_factor).
Changes:
- Add
alpha_maxtoget_ri_factor,get_global_ri_factor, andMicroSSIM.__init__, and re-exportALPHA_MAX_DEFAULT. - Increase the default bracketing cap from
1e3to1e6and validatealpha_maxeagerly. - Add tests for
alpha_maxbehavior/forwarding and for “pinned ri_factor skips fit()” usage inMicroSSIM/MicroMS3IM.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
cubic/metrics/microssim/ri_factor.py |
Adds alpha_max plumbing/default and updates bracketing behavior + docs. |
cubic/metrics/microssim/micro_ssim.py |
Adds alpha_max to constructor, validates/stores it, forwards to global fit. |
cubic/metrics/microssim/__init__.py |
Re-exports ALPHA_MAX_DEFAULT at the subpackage level. |
tests/metrics/microssim/test_ri_factor.py |
Adds coverage for alpha_max validation, default behavior, and forwarding. |
tests/metrics/microssim/test_micro_ssim.py |
Tests constructor alpha_max propagation and pinned-ri_factor scoring path. |
tests/metrics/microssim/test_micro_ms3im.py |
Tests pinned-ri_factor scoring and validation inherited from MicroSSIM. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Eager-validate alpha_max in get_global_ri_factor before the per-slice compute_ssim_elements loop (avoids O(N) wasted work on bad input). - Make alpha_max keyword-only in get_ri_factor / get_global_ri_factor / MicroSSIM.__init__ so positional misuse can't bind silently. - Document the power-of-2 probe granularity in the alpha_max docstring (effective largest probe = floor of log2(alpha_max)). - Document that get_parameters() intentionally does NOT round-trip alpha_max (mirrors upstream MicroSSIM's key set verbatim). - Tighten pinned-ri_factor score-equality tests from `< 1e-12` to `==` to express the bit-exact invariant — same params + same inputs through the same code path must produce the same float, not just a close one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Symmetric mirror of the alpha_max work: the bracket solver's left-side floor is now a public kwarg on get_ri_factor / get_global_ri_factor / MicroSSIM.__init__ (default 1e-6, bumped from the old hardcoded 1e-3). Use case: heavily up-scaled predictions (pred ~ 1e5 * gt) push the optimum to alpha ~ 1e-5, below the old 1e-3 floor; the bumped default now succeeds, and an explicit alpha_min raises cleanly when the floor is set above the optimum. Refactored validation into a shared _validate_alpha_bounds helper so get_ri_factor / get_global_ri_factor / MicroSSIM.__init__ all enforce the same contract: alpha_min in (0, 1), alpha_max > 1, both finite, both keyword-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ailure The doubling/halving bracket walks at powers of 2, so it could miss a sign change in (last_probe, alpha_max] (or [alpha_min, last_probe)) when the cap isn't itself a power of 2 — e.g. with the default alpha_max=1e6 the last probe is 2**19 = 524288, leaving (524288, 1e6] uncovered. Now probe the cap itself once if the doubling/halving schedule overshoots it, so the full requested interval is actually explored before raising RuntimeError. Caught by Copilot review on cubic/metrics/microssim/ri_factor.py:209. Also adds MicroMS3IM(alpha_min=..., alpha_max=...) inheritance test per Sourcery's overall feedback, pinning the contract that the new kwargs work uniformly across the class hierarchy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
alpha_minandalpha_maxkwargs toget_ri_factor/get_global_ri_factor/MicroSSIM.__init__so callers can lift / lower the bracket caps for pathological fits without monkey-patching internals.alpha_max1e3 → 1e6 (covers heavily down-scaled predictions, α* > 1e3) andalpha_min1e-3 → 1e-6 (covers heavily up-scaled predictions, α* < 1e-3).MicroSSIM(ri_factor=...)/MicroMS3IM(ri_factor=...)constructor path with tests pinning the use case: load per-(model, organelle) α calibrated once, reuse across all evaluation calls — no re-fit.ALPHA_MIN_DEFAULTandALPHA_MAX_DEFAULTfromcubic.metrics.microssimso callers can reference the cap defaults at a stable import path.MicroSSIM.__init__andget_global_ri_factor(out-of-range / NaN / ±inf raise immediately instead of after an expensive per-slice element pass). Shared_validate_alpha_boundshelper enforces the same contract everywhere.*,marker) to prevent silent positional misuse.Test plan
pytest tests/metrics/microssim/— 132 passed, 1 skippedpytest tests/metrics/— full metrics suite green (no regression in adjacent metrics)ruff check,ruff format --check,mypy cubic/clean🤖 Generated with Claude Code