AllenNeuralDynamics / AllenNeuralDynamics/aind-ophys-utils

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

未关闭
#79 0 条评论 0 个 reaction 已指派 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 摘要。