Skip to content

fix: PositionsLH penalty silently zero when positions contain inf rows - #765

Merged
Jammy2211 merged 1 commit into
PyAutoLabs:mainfrom
samlange04:fix/positions-lh-non-finite
Oct 7, 2026
Merged

Jammy2211 merged 1 commit into
PyAutoLabs:mainfrom
samlange04:fix/positions-lh-non-finite

Conversation

@samlange04

Copy link
Copy Markdown
Collaborator

Summary

Discussion: https://github.com/orgs/PyAutoLabs/discussions/30

The position penalty chained from a previous result via positions_likelihood_from did nothing whenever the positions held (inf, inf) rows, which the point solver's fixed-length output always has. PositionsLH.log_likelihood_penalty_from takes a plain xp.max over the traced separations; an inf position traces to nan, the max is nan, nan > threshold is False, and the penalty is zero for every model. output_positions_info and positions_threshold_from are nan-safe, so positions.info reported the true (violating) separation while the likelihood never saw it. In an HST SLaM fit this let source_pix[1] settle on a demagnified theta_E = 0.37" solution with a 2.38" source-plane separation against a 0.2" threshold.

This PR drops non-finite rows in PositionsLH.__init__ (numpy, at construction, so the JAX path sees a clean static grid) and raises PositionsException when fewer than two finite positions remain, extending the existing length-one check. A comment at the site explains why.

Not changed, open to discussion: positions_likelihood_from could also strip the padding before building the object, and the penalty could use a nan-safe max. A nan-safe max would also hide a real position that traces to nan, so filtering at construction is the narrower fix.

API Changes

  • PositionsLH now silently ignores non-finite positions. Previously they disabled the penalty, so no working configuration changes behaviour.
  • PositionsException is now raised for fewer than two finite positions, not only for exactly one. The message reports how many non-finite rows were dropped.

Test Plan

  • New test__non_finite_positions_are_dropped_and_the_penalty_still_fires: images at +/-1" padded with inf rows; the penalty is 0 at theta_E = 1" and fires at 0.3". Red on main.
  • New test__fewer_than_two_finite_positions_raises.
  • Every test file referencing PositionsLH / positions_likelihood: 61 passed (macOS, Python 3.13).
  • test_autolens/analysis, point, imaging: 381 passed, 1 xfailed, 1 failed. The failure (point/triangles/test_static_lattice_jax.py::test__source_on_a_step_0_vertex_returns_the_two_true_images) fails identically on unmodified main and does not touch PositionsLH.
  • Reproduced with the real J0008-0004 positions and fitted mass: max separation nan before, 2.384" after.

🤖 Generated with Claude Code

Result.positions_likelihood_from solves the previous model for its multiple
images with the point solver, whose output is fixed-length and padded with
(inf, inf). PositionsLH took a plain max over the traced separations: an inf
row traces to nan, the max is nan, `nan > threshold` is False, and the penalty
is zero for every model. output_positions_info uses a nan-safe max, so
positions.info reports the true separation while the likelihood never sees it.

In a SLaM fit of SLACS J0008-0004 this disabled the penalty from source_pix[1]
on: 2 solved images + 18 inf rows, and the pixelized fit walked to theta_E 0.37"
(parametric fit 1.47", Auger+09 1.16") with a source-plane separation of 2.38"
against a 0.2" threshold.

PositionsLH now drops non-finite rows on construction, and raises
PositionsException if fewer than two finite positions remain (it already raised
for one).

Tests: inf-padded positions keep the two real images and the penalty fires on a
model that splits them (red without the fix); one finite position among inf rows
raises.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Jammy2211
Jammy2211 merged commit dcfa5f0 into PyAutoLabs:main Oct 7, 2026
4 checks passed
@samlange04
samlange04 deleted the fix/positions-lh-non-finite branch October 16, 2026 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants