AllenNeuralDynamics / AllenNeuralDynamics/aind-ophys-utils

sigma_anneal_steps: 1 and 2 behave identically (should count intermediate points, not total)

オープン
#79 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
Python
スター
0
フォーク
0
PR マージ指標
30日以内にマージされた PR はありません

説明

## Problem

`sigma_anneal_steps` is passed directly as `num` to `np.geomspace(start, stop, num)`, which includes both endpoints. That makes `1` and `2` behave identically -- both just return `[start, stop]`, a single jump with no intermediate value. Graduated annealing only actually starts at `3+`. A "steps" parameter where two adjacent integer values do the exact same thing is a real footgun for anyone using it.

Found while writing documentation for `aind-ophys-dff` (AllenNeuralDynamics/aind-ophys-dff#23) -- not an active bug report, no observed failure, just a confusing API surfaced by trying to explain it clearly.

## Proposed fix

Draft implementation already exists: #78 (currently a draft, not ready to merge).

Redefine `sigma_anneal_steps` to count only the *intermediate* values between the loose starting scale and the tight target: `np.geomspace(start, stop, sigma_anneal_steps + 2)`.

- `0` -> no intermediate points (single jump) -- matches the *old* `1` or `2`.
- `1` -> one intermediate point -- matches the *old* `3`.
- `2` -> two intermediate points -- matches the *old* `4` (today's default).

Defaults in `nonlinear_fit` and `fit_baseline` would move from `4` to `2`, preserving current out-of-the-box behavior exactly. Migration for any explicit override: `new_value = max(0, old_value - 2)`.

## Why this is filed as an issue instead of merged now

This is a breaking change to a parameter with its own downstream defaults in two other repos (`aind-ophys-dff-library`, `aind-ophys-dff`), and isn't blocking anything on the current critical path (the Suite2p v1.1 / Cellpose 4.0 extraction migration and its dependency chain: AllenNeuralDynamics/aind-ophys-extraction#42, #77, #72, AllenNeuralDynamics/aind-ophys-dff#22, AllenNeuralDynamics/aind-ophys-nwb#79). Landing it now would add another moving part to an already-tangled merge sequence for a problem that isn't causing failures. Revisit once that chain has settled.

If/when this is picked up: also update AllenNeuralDynamics/aind-ophys-dff#23, which currently documents the *old* semantics.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。