Skip to content

test: pin FitEllipse.points_from_major_axis_from masked-loop behaviour #394

@Jammy2211

Description

@Jammy2211

Overview

Pin the current behaviour of FitEllipse.points_from_major_axis_from's 300-iteration mask-rejection loop with new unit tests in test_autogalaxy/ellipse/test_fit_ellipse.py. Step 3 of 7 in the ellipse_fitting_jax feature decomposition (PyAutoPrompt/z_features/ellipse_fitting_jax.md). Prompt 6 will rewrite this loop with a JAX-friendly oversample-then-NaN-mask approach; these tests are the regression target it must reproduce.

Plan

  • Add tests covering the four branches of the masked-points loop at autogalaxy/ellipse/fit_ellipse.py:81-134:
    • Zero-masked (mask doesn't overlap the ellipse — loop short-circuits).
    • Under-masked / trim path (mask drops a small fraction; loop trims via unmasked_indices[number_of_extra_points:]).
    • Over-masked / extra-points path (mask drops enough to force re-call with n_i=i for higher angular resolution).
    • Unreachable (ValueError after 300 iterations).
  • Cover both the with-multipole and without-multipole code paths (the inner for multipole in self.multipole_list: block at lines 113-120 is only hit when multipoles are present).
  • Pin numerical reference values for at least one mask configuration via np.testing.assert_allclose(rtol=1e-12) — the strongest regression target for the prompt-6 rewrite.
  • All tests numpy-only (PyAutoGalaxy/CLAUDE.md "Never use JAX in unit tests" rule); JAX parity is checked in workspace_test scripts.
  • Tests use explicit aa.Mask2D(mask=[...]) arrays (not Mask2D.circular) so the masked region is deterministic.
Detailed implementation plan

Affected Repositories

  • PyAutoGalaxy (primary)

Work Classification

Library

Branch Survey

Repository Current Branch Dirty?
./PyAutoGalaxy main clean

Suggested branch: feature/ellipse-fit-masked-loop-tests
Worktree root: ~/Code/PyAutoLabs-wt/ellipse-fit-masked-loop-tests/ (created later by /start_library)

Implementation Steps

  1. Add fixtures at the top of test_autogalaxy/ellipse/test_fit_ellipse.py:
    • imaging_30x30: 30x30 ag.Imaging with pixel_scales=1.0, data and noise-map of ones (small synthetic).
    • Four masks (explicit aa.Mask2D(mask=np.full(..., False), pixel_scales=1.0) with selective True patches) tuned for each branch.
  2. test__points_from_major_axis__zero_masked — apply a mask that's all-False across the ellipse footprint. Build FitEllipse and call _points_from_major_axis. Assert points.shape[0] == ellipse.total_points_from(pixel_scale). Assert behaviour matches the unmasked path byte-for-byte by comparing against FitEllipse(dataset=unmasked, ellipse=ellipse_same)._points_from_major_axis with np.testing.assert_allclose(rtol=1e-12).
  3. test__points_from_major_axis__under_masked_trim — mask drops ~10% of ellipse perimeter points (e.g. mask a single row that the ellipse barely crosses). Assert points.shape[0] == total_points_required. Pin the returned points as a hard-coded reference via np.testing.assert_allclose(rtol=1e-12).
  4. test__points_from_major_axis__over_masked_extra_points — mask drops enough points to force the loop to re-call ellipse.points_from_major_axis_from(..., n_i=i). Use a smaller-than-ellipse mask. Assert points.shape[0] == total_points_required after the loop converges. Pin a smaller spot-check (e.g. points[0], points[-1]) since the full array might be sensitive to the exact iteration count.
  5. test__points_from_major_axis__unreachable_raises — mask that geometrically excludes a large arc of the perimeter (e.g. major_axis far outside the mask radius). pytest.raises(ValueError, match="attempted to add over 300 extra points") matching the existing wording at fit_ellipse.py:124-132.
  6. test__points_from_major_axis__with_multipole_under_masked — same as step 3 but with multipole_list=[ag.EllipseMultipole(m=4, multipole_comps=(0.05, 0.0))] to exercise the inner multipole branch at lines 113-120.

Key Files

  • test_autogalaxy/ellipse/test_fit_ellipse.py — append the six new tests + fixtures. Existing tests untouched.
  • autogalaxy/ellipse/fit_ellipse.py:81-134 — read-only reference for the loop logic being pinned.

Testing Approach

  • python -m pytest test_autogalaxy/ellipse/test_fit_ellipse.py -v from the worktree (after sourcing activate.sh). Must pass including the new tests.
  • Run twice — reference values must be byte-stable (scipy RegularGridInterpolator is deterministic, but verify).

Original Prompt

Click to expand starting prompt

Step 3 of the ellipse-JAX series. The 300-iteration mask-rejection loop in FitEllipse.points_from_major_axis_from (@PyAutoGalaxy/autogalaxy/ellipse/fit_ellipse.py:81-134) is the single nastiest piece of imperative code in the ellipse module — Python for loop, dynamic shape changes via points = points[unmasked_indices], scipy interpolator calls, and a raise ValueError after 300 iterations. Prompt 6 will rewrite it for JAX. Before that we need unit tests in @PyAutoGalaxy/test_autogalaxy/ellipse/test_fit_ellipse.py that pin the current behaviour, so when the rewrite goes in we know nothing has shifted.

Please:

  1. Add (or extend) tests in @PyAutoGalaxy/test_autogalaxy/ellipse/test_fit_ellipse.py covering the four exit paths of the loop:

    • Zero-masked: a mask that doesn't overlap the ellipse points. Assert points.shape[0] == ellipse.total_points_from(pixel_scale) and that the loop exits on the first iteration via the total_points_required == total_points - total_points_masked branch.

    • Under-masked (trim path): a mask that drops a small fraction of points (e.g. ~10%). Assert the returned points has exactly total_points_required rows and that the trimmed indices are the latest ones in unmasked_indices (the slicing is unmasked_indices[number_of_extra_points:]).

    • Over-masked (extra-points path): a mask that drops enough points to force a re-call of ellipse.points_from_major_axis_from(..., n_i=i) with a higher angular resolution. Assert the loop runs for at least one iteration and that the final points.shape[0] == total_points_required.

    • Unreachable (ValueError): a degenerate mask where no i ≤ 300 satisfies the constraint. Use pytest.raises(ValueError) and assert the error message matches the existing wording in fit_ellipse.py.

  2. Use small Mask2D shapes (e.g. 30x30) so the tests run fast. Build the mask explicitly via aa.Mask2D(...) rather than Mask2D.circular, so the masked region is deterministic and the tests are robust to changes in the circular-mask helper.

  3. Cover both with and without multipole_list. The multipole branch goes through the inner for multipole in self.multipole_list: block at lines 113-120 — add at least one test that hits this with EllipseMultipole(m=4, multipole_comps=(0.05, 0.0)).

  4. Pin numerical reference values for at least one mask configuration: capture the returned points array via np.testing.assert_allclose(points, expected, rtol=1e-12) against a hard-coded reference. This is the strongest regression test — when prompt 6 swaps the loop for a JAX-friendly oversample-then-mask approach, the new path should reproduce these numbers (or the test must be re-pinned with a written justification).

  5. Follow @PyAutoGalaxy/CLAUDE.md "Never use JAX in unit tests" — these tests stay numpy-only. The cross-numpy/JAX parity check happens in the workspace_test scripts from prompt 2.

  6. Test bar: python -m pytest test_autogalaxy/ellipse/test_fit_ellipse.py -v passes, including the new tests.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions