AllenNeuralDynamics / AllenNeuralDynamics/aind-ophys-utils

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

Aberta
#79 0 comentários 0 reações 0 responsáveis Ver no GitHub
Linguagem predominante
Python
Estrelas
0
Forks
0
Métricas de merge de PRs
Nenhum PR com merge em 30d

Descrição

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

Guia de contribuição

Nenhum guia de contribuição indexado para este repositório

Avaliação

Esta issue ainda não foi avaliada.

Receba novas issues na sua caixa de entrada

Um resumo curto de issues do GitHub para quem está começando.