DM-54830#24
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
==========================================
+ Coverage 92.81% 93.34% +0.52%
==========================================
Files 49 49
Lines 6013 6386 +373
==========================================
+ Hits 5581 5961 +380
+ Misses 432 425 -7 ☔ View full report in Codecov by Sentry. |
Bumps pre-commit-hooks (v4.4.0->v6.0.0), black (23.1.0->26.3.1), isort (5.12.0->8.0.1), and flake8 (6.0.0->7.3.0). Updates black's language_version pin from python3.10 to python3.12 to match the repo's current development environment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Documents 30 findings (1 critical, 4 high, 8 medium, 17 low) across the four core subsystems, organized by area with cross-cutting issues deduplicated. Pair-coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a Status Tracking table to the top of the audit so each finding has a clear state (Open / Fixed / Discarded / Discuss). Records the four findings @fred3m marked as intentional design choices (I-5, K-4, D-3, D-11) and the two needing further investigation (O-4, D-4). Expands D-4 with the results of investigating the /2 factor in the detection sigma: traced to commit 33ee0b4 and confirmed empirically that it calibrates the detection image noise std to ~1.0 for the LSST 6-band coadd, so that peak_thresh/footprint_thresh values mean what they say in sigma units. Pair-coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fred3m
force-pushed
the
tickets/DM-54830
branch
3 times, most recently
from
2d4f3a3 to
837ab78
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements a broad set of code-audit-driven fixes across wavelet support estimation, initialization, optimization parameter copying/updates, FFT caching behavior, and documentation/CI support—backed by substantial new regression test coverage.
Changes:
- Fix and harden multiple numerical/algorithmic behaviors (wavelet support, parametric gradients, ADAM-family updates, bbox slicing).
- Add FFT caching controls (opt-in at
Observation.convolve, configurable at FFT primitives) plus deprecate/neutralize unusednormalizeargs with warnings. - Add regression tests for the above and update docs + doc-build tooling/workflows.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_wavelet.py | Adds regression tests for ground/space multiresolution-support correctness and reproducibility. |
| tests/test_parameters.py | Adds coverage for copy semantics, prox=None update, optimizer variants, and AdamX first-step behavior. |
| tests/test_operators.py | Adds coverage for even-shape centering behavior in uncentered_operator symmetry path. |
| tests/test_observation.py | Pins Observation.convolve(cache=...) default and opt-in caching behavior. |
| tests/test_models.py | Adds finite-difference tests for parametric model gradients (Sersic/Gaussian variants). |
| tests/test_initialization.py | Updates trimming/padding expectations and adds initialization regression tests (PSF boundary, div-by-zero spectra, fallbacks). |
| tests/test_image.py | Strengthens comparison-operator tests to distinguish strict vs non-strict behavior. |
| tests/test_fft.py | Adds tests for FFT cache controls and normalize deprecation warnings. |
| tests/test_detect.py | Adds detection boundary-condition tests for min_area and min_pixel_detect. |
| tests/test_component.py | Adds regression tests for positivity enforcement and deepcopy memo preservation. |
| tests/test_bbox.py | Adds regression test for rejecting negative origins in Box.slices. |
| python/lsst/scarlet/lite/wavelet.py | Fixes multiband reconstruction shape naming, adds RNG plumbing, and corrects iteration thresholding + ground sigma estimation. |
| python/lsst/scarlet/lite/parameters.py | Fixes copy/deepcopy propagation, AdamX first-iteration factor, and prox=None handling in Adaprox update. |
| python/lsst/scarlet/lite/operators.py | Tightens fast-path conditions for uncentered symmetry and clarifies symmetry semantics in docstring. |
| python/lsst/scarlet/lite/observation.py | Adds cache parameter to convolve and forwards it to FFT convolution. |
| python/lsst/scarlet/lite/models/parametric.py | Fixes multiple analytical gradients (log base, sigma scaling, size-parameter factor). |
| python/lsst/scarlet/lite/models/fit_psf.py | Adds cache plumbing and opts into caching in the fitting hot loop. |
| python/lsst/scarlet/lite/io/source_base.py | Adds default source_type value for base persistence dataclass. |
| python/lsst/scarlet/lite/io/component.py | Adds default component_type value for base persistence dataclass. |
| python/lsst/scarlet/lite/io/blend_base.py | Adds default blend_type value for base persistence dataclass. |
| python/lsst/scarlet/lite/initialization.py | Removes trim-mutation + double-padding behavior, fixes PSF extraction origin, guards divisions, and hardens init fallbacks. |
| python/lsst/scarlet/lite/image.py | Fixes Image strict comparison operator dispatch and docstrings. |
| python/lsst/scarlet/lite/fft.py | Adds logging, cache controls, and deprecates unused normalize param with warnings. |
| python/lsst/scarlet/lite/detect.py | Fixes union docstring and documents detection sigma calibration caveats. |
| python/lsst/scarlet/lite/detect_pybind11.cc | Fixes min_area boundary condition (>= instead of > pre-filter). |
| python/lsst/scarlet/lite/component.py | Enforces positivity regardless of bg-thresholding and fixes deepcopy memo handling. |
| python/lsst/scarlet/lite/blend.py | Opts into convolution caching in hot paths and removes redundant ratio assignment. |
| python/lsst/scarlet/lite/bbox.py | Fixes negative-origin guard for .slices. |
| pyproject.toml | Configures pytest to ignore doc/conf.py during test runs. |
| doc/requirements.txt | Adds doc-build dependency set. |
| doc/lsst.scarlet.lite/changes.rst | Adds unreleased bug-fix notes for user-visible changes. |
| doc/documenteer.toml | Adds Documenteer config file expected by the guide template. |
| doc/conf.py | Switches to Documenteer guide config and disables problematic typehints extension. |
| doc/.gitignore | Updates ignored doc build artifacts. |
| audits/audit-2026-04-30.md | Adds the audit report and status tracking referenced by the regression tests/changes. |
| .pre-commit-config.yaml | Updates pre-commit hook versions and Python version for Black. |
| .github/workflows/build.yaml | Removes doctest-time Documenteer install from unit-test workflow. |
| .github/workflows/build_docs.yaml | Updates doc build workflow to use uv, doc requirements, and graphviz install. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
318
to
319
| sigma_i = np.std(noise * s) | ||
| if np.abs(sigma_i - last_sigma_i) / sigma_i < epsilon: |
Collaborator
Author
There was a problem hiding this comment.
This section implements the algorithm from Starck and Murtagh 1998. Evaluating and revising this algorithm could be reserved for a future ticket, but that is beyond the scope of the present ticket.
Comment on lines
+287
to
+288
| Protects against regressions in grad_circular_gaussian (audit | ||
| finding C-2). The forward model is morph = exp(-r2) where |
There was a problem hiding this comment.
I'm not sure comments like this age well. Are you keeping the audit file around post-merge? Unless there's versions of those, I think this might cause confusion in the future (this is a current failure mode of the way Claude Code comments, IMO - I see it and fight it in my code to)
Collaborator
Author
There was a problem hiding this comment.
So my intent is actually to keep the document post-merge. I created a new folder and added the audit with the date in the name. I could go either way with this. My thinking was that since most of the tests from now on will likely be written by an LLM, this protects the test in the future because the LLM will be able to reference the issue in the audit file.
Do you agree with that thinking or would you like me to remove them?
There was a problem hiding this comment.
Oh, I think keeping them is fine, but there's no versioning in the comment, so you don't know which audit it refers to in the future - that's my main point really. Probably OK for one, but how will you tell when there's a new C-1 in another file?
Comment on lines
+290
to
+291
| must scale with 1/(2*sigma**2). A previous version of the code used | ||
| a fixed factor of 2, ignoring sigma entirely. |
There was a problem hiding this comment.
Again, maybe this is OK because it's in a test, but "there used to be a bug here" is kind of a weird thing to merge. Notes as you go - for sure, but weird for merging IMO.
There was a problem hiding this comment.
(I made Claude Code go over everything and remove all these automatically btw, but I did have to explain what I didn't like and ask for that)
| # Many __init__ methods in scarlet_lite are typed but lack a Parameters | ||
| # section. sphinx_autodoc_typehints injects a synthetic Parameters section | ||
| # that collides with the class docstring, producing a SEVERE parse error. | ||
| # Disable the extension until __init__ docstrings are normalized. |
There was a problem hiding this comment.
Worth just getting Claude to do that normalization instead? No worries if not, just a thought.
Collaborator
Author
There was a problem hiding this comment.
Totally. I missed this.
Comment on lines
+351
to
+353
| if len(fft_shape) != len(axes): | ||
| msg = f"fft_shape self.axes must have the same number of dimensions, got {fft_shape}, {axes}" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
I don't if this could ever happen, but I was just pondering: if you lifted this check + raise above the if fft_key in self._fft: return code above, then if anything ever went wrong, you'd see if, whereas right now, the cache could technically (and like I said, I don't know if this is ever possible) hide a size mismatch.
Collaborator
Author
There was a problem hiding this comment.
I don't think that can happen. If you look above, the created from the fft_shape and axes as tuples, so they have to match. The only way around this is if the user manually set _fft using mismatched fft_shape and axes, in which case they get what they deserve. The property is hidden for a reason.
Comment on lines
+356
to
+357
| if cache: | ||
| self._fft[fft_key] = result |
There was a problem hiding this comment.
Was this supposed to use an LRU cache? Or are you OK with it being arbitrarily sized?
Collaborator
Author
There was a problem hiding this comment.
It's arbitrary sized but by default most of the methods that actually use it do not cache. See the note in the audit that explains the reasoning behind doing it this way.
grad_sersic used np.log10 in the gradient w.r.t. the Sersic index n where the natural logarithm is required. The Sersic profile is exp(-bn * (r^(1/n) - 1)), and differentiating r^(1/n) w.r.t. n yields r^(1/n) * (-1/n^2) * ln(r). Using log10 made the gradient off by a factor of 1/ln(10) ~ 0.434, so updates to n were systematically ~2.3x too small. Adds a finite-difference regression test that holds bn fixed (matching the analytical formulation; the dbn/dn * (r^(1/n) - 1) term involving the inverse incomplete gamma function is an intentional omission and not addressed here). Verified the test fails on the buggy version. Pair-coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
grad_circular_gaussian computed the gradient w.r.t. (y0, x0) using a
fixed factor of 2, which was the gradient of exp(-((x-x0)**2 + (y-y0)**2))
-- a different forward model. The actual circular_gaussian forward model
is exp(-r2) with r2 = ((x-x0)/(2*sigma))**2 + ((y-y0)/(2*sigma))**2, so
the gradient must scale by 1/(2*sigma**2). The sigma argument was
accepted but ignored.
Effect: position gradients were off by 4*sigma**2. For a typical PSF
sigma of 0.8, updates to (y0, x0) were ~2.5x too large; for sigma=2 they
were 16x too large. Adaptive optimizers (AdaProx/ADAM) would partially
compensate, but fixed-step optimizers like FISTA would not.
Adds a finite-difference regression test across sigma in {0.6, 0.8, 1.5,
3.0}; verified it fails on the buggy version.
Pair-coded with @fred3m.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Use a dated filename so future audits can coexist cleanly, and put the file in a dedicated audits/ directory so engineering-internal audit docs don't end up in the Sphinx build under doc/. Pair-coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous guard
if np.any(self.origin) < 0:
is always False, because np.any(...) returns a bool and bool < 0 is
always False (True == 1, False == 0). Negative origin coordinates
therefore reached NumPy unchecked and were silently interpreted as
indices counting from the end of the array.
Replaced with a per-element check using Python's built-in any over a
generator -- self.origin is a small tuple of ints, so no point
allocating a numpy array.
Adds a regression test covering all-negative, mixed, and the
positive-then-negative case the buggy guard most clearly missed.
Verified the test fails on the buggy version.
Pair-coded with @fred3m.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When comparing two Image objects, __gt__ dispatched operator.ge (>=)
and __lt__ dispatched operator.le (<=), so equal-valued pixels were
incorrectly included in the strict comparison result. The scalar
comparison branches were already correct. The docstrings were also
copy-paste wrong ("greater than or equal" on __gt__).
The existing arithmetic tests already exercised gt/lt between Images,
but the random int-vs-float test data essentially never had equal
pixels, so the buggy >= and > gave identical results. Strengthening
the 2D and 3D arithmetic test data to force a few pixels to be
exactly equal makes the existing _binary_operation_test machinery
catch this and any future strict-vs-non-strict comparison regressions.
Verified the strengthened tests fail on the buggy version.
Pair-coded with @fred3m.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FactorizedWaveletInitialization.init_source had two paths that left
``components`` either unbound or empty:
1. The single-component branch (nbr_components < 2) only assigned
``components`` when get_single_component returned non-None. When
it returned None, ``return Source(components)`` raised
UnboundLocalError.
2. The two-component fallback (one of bulge/disk was None) had the
same problem.
3. The two-component success path could produce ``components = []``
when both bulge and disk spectra were cut, yielding a
Source([]) -- valid Python but problematic downstream.
Replaced with a sentinel ``components = None`` at the top of the
method and a single ``if not components`` fallback at the bottom that
falls back to a PSF component (the most conservative non-empty model
we can return). This collapses all three failure modes into one
explicit, easy-to-audit decision point.
Adds a regression test that mocks both get_single_component (returning
None) and multifit_spectra (returning all-zero spectra) to drive
init_source through every failure path; verified the test reproduces
the original UnboundLocalError on the buggy version.
Pair-coded with @fred3m.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
trim_morphology grew its returned bbox by ``padding`` and the only caller, init_monotonic_morph, then grew it by ``padding`` again at the bottom of the function. For the default padding=5 every monotonic init wound up with 10 pixels of padding instead of 5. Removed the ``padding`` parameter from trim_morphology entirely; it now returns a tight bbox around the non-zero pixels and leaves padding to the caller. trim_morphology isn't part of the public API (__init__.py doesn't re-export it), so the signature change is internal. Also fixed a pre-existing docstring typo (``thresh`` -> ``threshold``). The existing test_init_monotonic_mask and test_init_monotonic_weighted tests already had a "specify parameters" case with threshold=0.2, but both used padding=0 -- which makes any padding bug invisible. Strengthening those cases to use padding=2 in addition to threshold=0.2 makes the trim_morphology + post-trim-pad interaction observable. Verified both fail on the buggy version. The corresponding test_trim_morphology was updated to match the new tight-bbox contract. Pair-coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
In `get_psf_component`, when a source's PSF box was clipped by the observation boundary, the Image was constructed with the *intersection* origin instead of the original PSF origin. The slice then extracted the top-left of the PSF rather than the surviving region, leaving the component spatially misaligned with the source center. Use the original PSF box origin for the Image so the slice extracts the correct sub-region. Also reject centers that fall outside the observation up front, so the spectrum lookup can no longer wrap a negative local index into NumPy data. Audit finding I-3. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
In `get_single_component`, dividing the per-band image by the convolved detection coadd produced `inf` (or `nan` for 0/0) wherever the convolved image was zero at the source center. The trailing `spectrum < 0` check caught neither, so faint or masked sources were initialized with non-finite flux. Use `np.divide` with a `where=conv > 0` mask so the spectrum stays at 0 when there is no convolved signal to divide by, rather than producing inf/nan or — under the audit's suggested `np.maximum(conv, 1e-20)` alternative — an arbitrarily large finite value. Audit finding I-4. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`morph_images[b]` is already 2D, so `np.vstack` is a no-op. Replace with the 2D slice directly so the design matrix construction reads as the simple transpose it always was. Audit finding I-6. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`init_source` aliased the disk morph to `component.morph`, so the in-place threshold clip mutated the single-component model that was just fit. The shared reference was safe today only because nothing read `component.morph` after the split, but it would silently corrupt any future use. Audit finding I-7. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The in-place ``morph[~mask] = 0`` was an undocumented side effect. Replace with ``np.where`` to return a freshly-allocated trimmed array and leave the caller's input untouched, matching the expectation set by the function name and signature. Audit finding I-8. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The bulge branch tested ``np.sum(bulge_spectrum != 0)`` (count of non-zero bands) while the disk branch tested ``np.sum(disk_spectrum) != 0`` (total flux). Both are equivalent for the non-negative spectra ``multifit_spectra`` returns, but the inconsistency invites future divergence. Use ``np.any(spectrum != 0)`` in both branches. Audit finding I-9. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
In `get_psf_component`, dividing the per-band image value by `self.psf_spectrum` produced `inf` (or `nan` for 0/0) for any band with a degenerate model PSF whose central pixel is zero. The trailing `spectrum < 0` mask did not catch either, so the PSF component would be initialized with non-finite flux. Apply the same `np.divide(..., where=denom > 0, out=zeros)` pattern used for the I-4 fix, so a degenerate band yields zero flux instead of inf. Audit finding I-10. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`update` called `self.prox(_x)` unconditionally and raised `TypeError` when constructed without a prox, even though the constructor and ``FistaParameter`` both treat ``prox=None`` as supported. The in-place gradient step already updates ``self.x`` (since ``_x`` aliases it), so when no prox is provided we simply skip the rebind. Audit finding O-1. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
At ``it=0`` the formula read ``b1[it-1] = b1[-1]``. With the default ``SingleItemArray`` (constant ``b1``) this happens to return the right value, but a varying schedule would compute the factor from the *last* element of ``b1`` instead. Pin ``factor = 1.0`` on the first iteration, matching the AdamX paper at t=1 (no prior ``vhat`` to scale). Audit finding O-2. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`fft.match_kernel` and `fft.convolve` declared a `normalize` keyword that was never referenced — passing it had no effect either way. Switch the default to a sentinel ``None`` and emit the per-argument deprecation log used elsewhere in the package (``bg_thresh`` in ``trim_morphology``, ``convolved`` and ``thresh`` in ``FactorizedInitialization``), scheduled for removal after v31.0. Audit finding O-3. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`ratio` is initialized via `np.zeros`, then only the ``denominator != 0`` positions are written. Re-zeroing the ``denominator == 0`` positions afterwards is a no-op. Audit finding O-5. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The base expression in `grad_major`/`grad_minor` was already ``d(r**2)/d(major)``, while the bases in `grad_x0`, `grad_y0`, and `grad_theta` were ``(1/2) * d(r**2)/d(...)``. The post-multiplications ``*= 2`` (when `use_r2=True`) and ``*= 1/r`` (when `use_r2=False`) all assume the half-r**2 convention, so both branches were returning twice the correct gradient for the size parameters. Change the bases to ``-1/major * _xa**2`` and ``-1/minor * _yb**2`` so ``grad_major`` and ``grad_minor`` follow the same convention as the other partial derivatives. Every parametric fit with size parameters (`grad_gaussian2`, `grad_sersic`) was taking 2x the intended optimizer step on sigma_y/sigma_x. Surfaced while adding the FD coverage flagged in audit O-6 — recorded as new finding C-5. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round out the FD coverage flagged in audit O-6: the C-1, C-2, and
C-5 fixes already added FD checks for `grad_sersic` (d/dn),
`grad_circular_gaussian`, and `grad_gaussian2`. This commit adds
the last two missing pieces:
* `test_grad_integrated_gaussian` — d/d{y0, x0, sigma}
* `test_grad_sersic_ellipse_params` — d/d{y0, x0, sigma_y, sigma_x,
theta} (sersic d/dn already covered by `test_grad_sersic_n_index`)
With this, every analytic gradient in `parametric.py` has an FD
regression test. Pair coded with @fred3m.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Each of the six phi/psi schemes (`adam`, `nadam`, `amsgrad`, `padam`, `adamx`, `radam`) was previously exercised only by a smoke loop that checked nothing about the result. Add a convergence test that runs each variant on a quadratic loss with a known optimum, asserting the parameter reaches the target within a small tolerance. Catches semantic regressions in any per-iteration update formula — the kind of bug fixed for O-2. Also corrects the audit summary and status counts that had drifted from the table since findings were added/recategorized. Audit finding O-7. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Base ``Parameter.__copy__`` and ``__deepcopy__`` rebuilt the copy with ``step=0`` (an int — not even callable, so any access to ``self.step`` on the copy would raise ``TypeError``) and dropped ``grad`` and ``prox`` entirely. Subclasses override these correctly, but copying a base ``Parameter`` left it non-functional for optimization. Carry ``_step``, ``grad``, and ``prox`` over to the copy after the default construction (``deepcopy`` versions for ``__deepcopy__``). Audit finding K-1. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Positivity (``morph[morph < 0] = 0``) was previously only enforced on the no-``bg_thresh`` branch. With ``bg_thresh`` active, only the ``spectrum * morph >= bg_thresh`` check ran, so a negative morph pixel survived whenever ``spectrum`` had a negative band — ``neg * neg`` is positive and can pass the threshold. Hoist the positivity guard above the if/else so it always runs, then apply the threshold conditionally. Audit finding K-2. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clarify that ``prox_sdss_symmetry`` enforces symmetry about the *geometric* center of the array — for odd-shaped axes that is the center pixel, for even-shaped axes it is the half-pixel offset between the two central pixels. Both are mathematically valid; the latter is just rarely what astrophysical callers want. Callers that need integer-pixel symmetry on an even-shaped array should go through ``prox_uncentered_symmetry``, which slices to an odd-shaped subregion before calling this helper. (See K-5 for the fix in that wrapper.) Audit finding K-3, reconsidered. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
``uncentered_operator``'s ``peak == cy and px == cx`` early-return called ``func`` on the full array. For even-shaped axes that delegated a half-pixel-offset symmetry to ``prox_sdss_symmetry`` even though the integer ``peak`` argument signals pixel-center intent. Tighten the early-return to also require both axes odd, so even shapes go through the slicing path that yields an odd-shaped subregion centered on the peak. The bug was surfaced while filling the K-5 coverage gap: previously only one even-shaped case ``(5, 10)`` was tested, and it was off-center, so the early-return path was never exercised on an even-shaped array. Audit finding K-5. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
``CubeComponent.__deepcopy__`` called ``self._model.copy()`` (a fresh allocation) instead of ``deepcopy(self._model, memo)``. When two components in an object graph shared an ``Image`` reference, the copy silently forked them into two independent images, violating the standard Python deepcopy contract. Use ``deepcopy(..., memo)`` for both ``self._model`` and ``self.peak``. Audit finding K-6. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The bounding-box area pre-filter at ``detect_pybind11.cc:332`` used strict ``>`` while the documented and actual-pixel-count check on the following lines uses ``>=``. A footprint occupying every pixel of its tight bounding box (e.g., a 2x2 filled square with ``min_area=4``) was rejected by the pre-filter before the true count check ran, so the ``>=`` semantics promised in the docstring did not hold for that boundary case. Change ``>`` to ``>=`` so the pre-filter matches the documented contract. Add a release note flagging the user-visible behavior change for anyone who tuned ``min_area`` against the previous cut. Audit finding D-1. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ``image_type='space'`` branch of ``get_multiresolution_support`` implements Starck & Murtagh's Multi-Resolution Support algorithm, which iteratively refines the global noise ``sigma_e`` from pixels that are insignificant at every scale. The threshold expression inside the loop, however, used the original input ``sigma`` instead of the iteratively refined ``last_sigma_i``. Each iteration produced the identical mask, so the new ``sigma_i`` was computed but never fed back into the threshold and the loop terminated after two iterations regardless of input. Use ``last_sigma_i`` in the threshold so each iteration's mask reflects the previous iteration's refined estimate, matching the ``ground`` branch (which already updates its per-scale ``sigma_j``). Audit finding D-2. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ``ground`` branch of ``get_multiresolution_support`` computed ``sigma_j = np.std(starlets * s.astype(int), axis=(1, 2))``, which zeros out the significant pixels and includes those zeros in the standard deviation. The added zeros pull the variance down, so the per-scale noise estimate was biased downward — by ~1% at the finest scale and up to ~7% at the coarsest scale in synthetic tests with a moderately bright signal region. Compute ``np.std`` per scale over the unmasked pixels only, with an explicit fall-through to ``0`` for scales where every pixel is significant (matching the pre-fix downstream behavior, which uses the ``sigma_j > 0`` cut to skip those scales). Note the small user-visible threshold change in the release notes. Audit finding D-5. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ``space`` branch drew its calibration noise realization from the global ``np.random`` state, so two identical calls returned different supports unless the caller had separately seeded the global RNG. Add an ``rng: np.random.Generator | None = None`` keyword. When ``None``, default to ``np.random.default_rng(0)`` so the default behavior is reproducible. Callers can still override for sensitivity studies. Audit finding D-6. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rename the loop variable to ``band_image`` so the function parameter remains accessible and the per-band slice is named distinctly from the multiband input. Audit finding D-7. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
In ``multiband_starlet_reconstruction`` the unpacked dimension names were swapped relative to numpy convention (second-to-last is height, last is width). Pure rename — both names were used to allocate the output array, so no functional change. Audit finding D-8. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Said "intersection" instead of "union" — copy-paste error from the preceding ``intersection`` method. Audit finding D-9. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds ``test_detect_footprints_min_pixel_detect`` exercising the multi-band detection filter: - Single-band input with ``min_pixel_detect=2`` filters out every pixel (each pixel is above zero in at most one band). - Two-band input where the two bands' signals don't spatially overlap also yields no footprints with ``min_pixel_detect=2``. - Sanity check with ``min_pixel_detect=1`` recovers the union of both bands' footprints. Other items under D-10 already had coverage (``remove_high_freq=True`` in the existing test_detect_footprints; the space-based wavelet branch via the D-2 and D-6 regression tests). Edge cases (empty images, all-NaN variance, boundary footprints) are deferred as per-failure follow-ups; recorded in the audit's developer comment. Audit finding D-10. Pair coded with @fred3m. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`Fourier` caches one FFT per shape on the underlying instance, so long-lived kernels held by `Observation` accumulate one entry per distinct image shape ever convolved. Downstream code calls `observation.convolve(...)` on per-source / per-component models at varying shapes, so the kernel `_fft` dict really does grow. Make caching opt-in by drawing the boundary at `Observation.convolve`, which now defaults to `cache=False`. The `fft` module retains its `cache=True` default so direct callers are unaffected. The five hot-loop full-blend sites in `Blend` and `FittedPsfBlend` opt in explicitly, preserving the speedup that motivates the FFT module. Co-Authored-By: @fred3m <noreply@anthropic.com>
The `/ 2` factor in `detect_footprints` is an empirical calibration that gives the detection image unit noise std for the LSST 6-band case with `remove_high_freq=True` — not a true noise estimate. Document the derivation inline (gen-2 B-spline smoothing reduces per-band noise by F = sum((h*h)**2) ~= 0.196, principled sigma is `median(sqrt(variance)) * F * sqrt(N_bands)`, which `/ 2` approximates within ~4% only for N=6) and flag the band-count caveat in the `peak_thresh` / `footprint_thresh` docstrings. The full numerical fix (proper chi^2 coadd + analytic calibration) is deferred to DM-54860 because it warrants independent mathematical review, and `detect_footprints` is not exercised by LSST science pipelines production runs. Co-Authored-By: @fred3m <noreply@anthropic.com>
The legacy documenteer[pipelines]<1.0 chain transitively imports lsst_sphinx_bootstrap_theme, which imports pkg_resources -- removed from setuptools 82.0.0 (2026-02-08). Both the doctest collection in the test job and the docs build job were failing with ModuleNotFoundError: No module named 'pkg_resources'. This adopts the modern Rubin docs stack used by daf_butler: * doc/conf.py: from documenteer.conf.pipelinespkg -> documenteer.conf.guide * doc/documenteer.toml: new file with project title and github_url * doc/requirements.txt: new file pinning documenteer[guide]>=2.0,<3.0 and lsst-sphinxutils from git * build.yaml + build_docs.yaml: install via -r doc/requirements.txt; modernize build_docs.yaml to uv + Python 3.13 + actions v6 * pyproject.toml: addopts="--ignore=doc/conf.py" so pytest's --doctest-modules doesn't try to import conf.py from the repo root (documenteer.conf.guide expects documenteer.toml in cwd) documenteer.conf.guide enables sphinx_autodoc_typehints with always_document_param_types=True. Several scarlet_lite __init__ methods are typed but lack Parameters sections; the extension's synthetic Parameters injection collides with class docstrings. Disable the extension via conf.py override until those __init__ docstrings are normalized; docs still render with types in the function signatures. Tested locally: pytest with --doctest-modules --doctest-glob="*.rst" passes 152 items; package-docs build succeeds with 5 pre-existing warnings (3 unknown py:module roles in getting_started.rst, 1 typo'd autosummary entry, 1 missing logo asset). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Newer mypy flags `cls.<thing>_type` access in ScarletBlendBaseData.register / ScarletComponentBaseData.register / ScarletSourceBaseData.register because the field is declared as an instance-only attribute on the base class. Subclasses always override it with a class-level default (e.g. `blend_type: str = BLEND_TYPE`), so the access works at runtime; an empty-string default at the base gives mypy the same view without changing behavior, since these classes are abstract and never instantiated directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The audit had stripped sphinx_autodoc_typehints out of the extension list in doc/conf.py because several type-hinted constructors lacked a Parameters section, which made the extension inject a synthetic param block that collided with the class docstring and aborted the build with a SEVERE docutils error. Rather than disable the extension (it supplies parameter *types* from the annotations so docstrings don't repeat them), re-enable it and fix the docstrings: - Add class-docstring `Parameters` sections (matching the Box/Image/ Observation convention, no inline types) to FistaParameter, AdaproxParameter, FixedParameter, FreeFormComponent, FittedPsfObservation, ComponentCube and SingleItemArray; relocate the misplaced/stale `Parameters` of CubeComponent and CartesianFrame from `__init__` to the class docstring; split ScarletModelData into `Parameters` (constructor args) and `Attributes` (class-level constants); document the MultiResolutionSupport dataclass fields under `Attributes`. - Fix non-standard section headers that docutils rejected as unexpected section titles once the extension was active: `Return`/`Parameter`/ `Results`/`Paramters` typos and the over-indented `Parameters` block in Image.project. Docs now build with no SEVERE/CRITICAL errors (remaining warnings are pre-existing and unrelated: getting_started.rst py:module roles, the `parameter` autosummary stub, and the sitemap html_baseurl config). 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.
Checklist
doc/changes