AllenNeuralDynamics / AllenNeuralDynamics/aind-ophys-utils

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

Đang mở
#79 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Python
Star
0
Fork
0
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

## 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)

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.