fix: PointSolver zero-image and coarse-precision crashes, plus two consumer gaps - #662
Merged
Merged
Conversation
Two realistic inputs crashed `PointSolver.solve` with errors naming nothing the caller controls. 1. A source-plane coordinate outside the region the image-plane grid tiles produces no images. `filtered_means` is then empty and the `Grid2DIrregular` built from it exposes `.array` as a bare list, so the `axis=1` reductions raised `numpy.exceptions.AxisError: axis 1 is out of bounds for array of dimension 1`. Zero images is a legitimate answer, so the solver now returns a correctly shaped empty grid and logs why. (The report expected a single image here; the tiling genuinely finds none -- the coordinate lies outside the tiled region.) 2. `n_steps` is `ceil(log2(scale / pixel_scale_precision))`, which goes negative once the requested precision is coarser than the initial triangle scale. The existing guard tested `== 0`, so negative values slipped through, `range(-1)` yielded no steps, and `steps[-1]` raised `IndexError: list index out of range`. The guard now tests `<= 0` and reports `pixel_scale_precision`, the triangle scale, and a workable value. It deliberately does not clamp to 1 step, which would silently solve at a precision the caller did not ask for. Reported by @rhayes777 in #531. Phase 1 of PyAutoArray#416 (epic PyAutoArray#415). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…o images Follow-up to de3b436, from an independent Codex review of that commit. Making `PointSolver.solve` return an empty grid rather than raising made two pre-existing consumer gaps reachable; both are fixed here. 1. `FitPositionsImagePairAll.chi_squared` returned NaN with no model positions: `n_permutations` is 0, so `-log(0)` is +inf while the permutation sum is -inf. `fitness.py` converts a NaN log-likelihood into `resample_figure_of_merit`, so the model was silently resampled instead of scored -- precisely what `no_image_residual` exists to prevent. `FitPositionsImagePair` and `FitPositionsImagePairRepeat` both already applied that floor; `PairAll` was the one sibling that did not. It now returns the same value they do (verified equal, not merely finite). Selected with `xp.where`, not a Python `if`: the model-position count is a traced value under `jax.jit`. The log is taken on a clamped count so the NaN never forms in the branch `where` discards. 2. `Result.image_plane_multiple_image_positions` invoked its inward-walk recovery only for exactly one image, so zero images fell through as an empty grid and `SourceMaxSeparation` reduced over it with `max()` on an empty sequence. Zero is the case that most needs the recovery; `== 1` -> `<= 1`. Also from the same review: the empty-result warning now fires on the final result, catching the second route to empty (all candidates rejected by `magnification_threshold`, where `_filter_low_magnification` preserves the array length and writes NaN rows). Corrects a comment that described `filtered_means` as a Python list -- it is an array -- and makes the `AbstractSolver` guard message shape-agnostic, since `ShapeSolver` shares it. Co-Authored-By: Claude Opus 5 <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.
Phase 1 of the @rhayes777 API-audit campaign (epic PyAutoArray#415, task PyAutoArray#416).
Fixes both
PointSolvercrashes reported in #531, plus two consumer gaps an independentreview surfaced.
Pairs with PyAutoArray#417 — merge that first (this branch's suite runs against it).
The two reported crashes
Both are inputs a user reaches on purpose.
1. Source outside the tiled region →
numpy.exceptions.AxisErrorNo triangle traces to the coordinate, so
filtered_meanshas shape(0, 2); the[pair for pair in ...]comprehension then yields[],Grid2DIrregular([]).arrayis a barelist, and the
axis=1reductions blew up.Zero images is a legitimate answer, so
solve()now returns a correctly-shaped empty grid andlogs why. (The report predicted a single image here. The tiling genuinely finds none — the grid
spans ±2″ and the source sits at (5,5) — so returning an empty grid is the honest answer rather
than fabricating one.)
2. Loose
pixel_scale_precision→IndexError: list index out of rangen_stepsisceil(log2(scale / pixel_scale_precision)), which goes negative once therequested precision is coarser than the initial triangle scale. The existing guard tested
== 0,so negatives slipped through,
range(-1)yielded no steps, andsteps[-1]raised.Now tests
<= 0and reportspixel_scale_precision, the triangle scale, and a workable value.Deliberately not clamped to 1 step — that would silently solve at a precision the caller
never asked for.
Two consumer gaps this exposed
Both predate this branch and were unreachable while
solve()crashed first. Found by anindependent Codex review of the first commit.
FitPositionsImagePairAll.chi_squaredreturned NaN with no model positions.n_permutationsis 0, so
-log(0)is+infwhile the permutation sum is-inf.fitness.py:262converts a NaNlog-likelihood into
resample_figure_of_merit— so the model was silently resampled instead ofscored, precisely what
no_image_residualexists to prevent.FitPositionsImagePairandFitPositionsImagePairRepeatboth already applied that floor;PairAllwas the one sibling thatdid not. It now returns the same value they do (asserted equal, not merely finite).
Selected with
xp.where, not a Pythonif— the model-position count is a traced value underjax.jit. The log is taken on a clamped count so the NaN never forms in the discarded branch.Result.image_plane_multiple_image_positionsrecovered only at exactly one image. Zero fellthrough as an empty grid and
SourceMaxSeparationreduced over it (max()on an empty sequence).Zero is the case that most needs the inward-walk recovery:
== 1→<= 1.API Changes
Behavioural change, no signature change.
PointSolver.solve(...)returns an empty(0, 2)Grid2DIrregularwhere it previouslyraised
AxisError. Callers already handling a variable image count need no change; anythingassuming ≥1 image now sees length 0 rather than an exception. Verified
.shape[0],len(),.in_listand re-wrapping all behave.solve(...)raises a clearerValueError(wasIndexError) whenpixel_scale_precisionexceeds the triangle scale. The guard lives on
AbstractSolver, soShapeSolvergains the samemessage.
FitPositionsImagePairAllgainsno_image_residual = 1.0e4, matching its siblings. Fits thatpreviously produced NaN now produce a finite, loudly-bad chi-squared.
WARNINGis logged wheneversolve()returns empty, namingmagnification_threshold.Downstream impact: none expected — every changed path previously raised or produced NaN.
Testing
all-candidates-filtered), a no-warning control, the 4-image control, and the
PairAllfloorasserted equal to both siblings.
PointSolverat precision0.001still returns its 4 images.Reported by @rhayes777. Closes #531.