Skip to content

[codex] Fix EllipseSample geometry sma cache reset#2280

Merged
larrybradley merged 2 commits into
astropy:mainfrom
hgao-astro:fix-isophote-sma-cache
May 29, 2026
Merged

[codex] Fix EllipseSample geometry sma cache reset#2280
larrybradley merged 2 commits into
astropy:mainfrom
hgao-astro:fix-isophote-sma-cache

Conversation

@hgao-astro
Copy link
Copy Markdown
Contributor

@hgao-astro hgao-astro commented May 28, 2026

Summary

Fixes #2105 by resetting EllipseGeometry attributes that are derived from sma when EllipseSample is constructed from an existing geometry and a new semimajor axis.

The root cause was that EllipseSample deep-copied the input geometry and overwrote geometry.sma, but left cached SMA-dependent values such as _area_factor, sector_angular_width, initial_polar_angle, and initial_polar_radius from the original geometry.

Changes

  • Moved SMA-dependent geometry initialization into a private helper on EllipseGeometry.
  • Called that helper after EllipseSample overrides the copied geometry sma.
  • Added a regression test that compares the copied-and-overridden geometry against a freshly constructed geometry with the target sma.

Validation

  • python -m pytest photutils/isophote/tests/test_sample.py
  • python -m pytest photutils/isophote/tests/test_geometry.py photutils/isophote/tests/test_sample.py

@hgao-astro hgao-astro marked this pull request as ready for review May 28, 2026 13:56
Copy link
Copy Markdown
Member

@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks, @hgao-astro! I added a changelog entry for this bugfix and will merge when CI passes.

@larrybradley larrybradley force-pushed the fix-isophote-sma-cache branch 2 times, most recently from d21ea5a to 9bede7e Compare May 29, 2026 18:13
@larrybradley larrybradley merged commit fc2f6d9 into astropy:main May 29, 2026
28 checks passed
@hgao-astro hgao-astro deleted the fix-isophote-sma-cache branch May 29, 2026 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

overwriting sma of EllipseGeometry in EllipseSample constructor

2 participants