Skip to content

feat: make the minimum MGE Gaussian sigma configurable (sigma_min) - #549

Merged
Jammy2211 merged 2 commits into
mainfrom
feature/mge-sigma-min
Aug 4, 2026
Merged

feat: make the minimum MGE Gaussian sigma configurable (sigma_min)#549
Jammy2211 merged 2 commits into
mainfrom
feature/mge-sigma-min

Conversation

@NiekWielders

Copy link
Copy Markdown
Collaborator

Summary

mge_model_from hard-coded the lower end of the log-spaced sigma list:

# The sigma values of the Gaussians will be fixed to values spanning 0.01 to the mask radius.
log10_sigma_list = np.linspace(-4, np.log10(mask_radius), total_gaussians)

So sigmas actually ran from 1e-4" (not the 0.01" the comment claimed) up to the mask radius, meaning the basis always spent Gaussians on scales far below the resolution of any real dataset.

This PR adds a sigma_min argument so the smallest Gaussian width can be set by the user, e.g. to the pixel scale or half the PSF FWHM:

ag.model_util.mge_model_from(mask_radius=3.0, total_gaussians=30, sigma_min=0.01)

Details

  • sigma_min: float = 1e-4 (arcsec), appended to the end of the signature so existing positional calls are unaffected. The default reproduces the previous -4 behaviour exactly.
  • Validated: ValueError if sigma_min <= 0 (log10 would blow up) or if sigma_min > mask_radius (the list would run backwards).
  • Stale comment corrected.

Testing

python -m pytest test_autogalaxy/analysis — 44 passed. New tests cover the default span, a user-set sigma_min (checked against the expected log-spaced values) and both validation errors.

🤖 Generated with Claude Code

NiekWielders and others added 2 commits August 4, 2026 14:44
`mge_model_from` hard-coded the lower end of the log-spaced sigma list at
10**-4 arcsec, so the basis always spent components on scales far below the
resolution of any real dataset (and the comment claimed 0.01 arcsec, which
it never was).

Add a `sigma_min` argument (default `1e-4`, preserving existing behaviour)
setting the smallest Gaussian width in arcseconds, validated to be positive
and no larger than `mask_radius`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…model_from

Follow-up to the `sigma_min` argument, covering three things:

- Regression tests asserting the DEFAULT sigma ladders of both
  `mge_model_from` (sigma_min=1e-4) and `mge_point_model_from`
  (sigma_min=0.01) reproduce the previously hardcoded `-4` / `-2`
  linspace values EXACTLY. Every fixed sigma feeds the PyAutoFit run
  identifier, so drift would orphan the output directories of existing
  fits. The assertions use `==` rather than `pytest.approx`, which only
  fails once the ladder has moved past the point the identifier changes.

- `mge_point_model_from` gains the same `sigma_min` argument (default
  0.01, reproducing the hardcoded `min_log10_sigma = -2.0` exactly),
  since it had the identical hardcoded floor and is the function that
  already receives `pixel_scales`.

- `sigma_min` docstring entry moved to match its position in the
  signature.

Verified bit-identical: `np.log10(1e-4)` is exactly -4.0 and
`np.log10(0.01)` exactly -2.0, so both linspace calls receive unchanged
endpoints. Diffed model-by-model against main across 1440 configurations
of mask_radius x total_gaussians x gaussian_per_basis x use_spherical x
centre prior modes: zero sigma differences, zero identifier differences.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Jammy2211

Copy link
Copy Markdown
Collaborator

Thanks @NiekWielders — good catch. The docstring promised 0.01" while the code laid the first Gaussian at 1e-4", so the basis was spending components ~2 dex below anything a real dataset resolves. The shape is right too: linear arcsec reads better at the call site than a log10 value, and appending the argument last keeps existing positional calls working.

One thing I checked before merging, because it's the risk that would bite hardest here: existing runs must not get a new unique_id. Every in-flight fit was run with the hardcoded -4, and if np.log10(1e-4) came back as anything other than exactly -4.0, every fixed sigma would shift in its last bits and orphan the output directory of every existing fit.

It's safe. np.log10(1e-4) is exactly -4.0 (-0x1.0000000000000p+2), so np.linspace receives unchanged endpoints. I diffed the model built by main against the model built by this branch across 1440 configurations — every mask_radius used in the workspace, total_gaussians in {2,5,10,20,30,40}, gaussian_per_basis, spherical/elliptical, and both centre-prior modes — and got zero bit-level sigma differences and zero PyAutoFit identifier differences.

I've pushed three follow-up commits to your branch rather than sending you round again:

1. Regression tests locking both default ladders. The invariant above is worth having in CI, so a future edit to this line can't quietly re-space the Gaussians of every archived run. The assertions use exact == rather than pytest.approx, deliberately — measuring it, approx(rel=1e-8) only starts failing once the ladder has drifted by a relative ~1e-7, which is already past the point where the identifier moves, whereas exact equality catches drift from ~1e-15. (The identifier itself quantizes floats at RESOLUTION = 1e-8, so it is not literally last-bit sensitive — but exact equality is the stronger guarantee and costs nothing.)

2. mge_point_model_from gets the same sigma_min. It sits ~60 lines below in the same module with the identical hardcoded floor (min_log10_sigma = -2.0), and it's the function that already receives pixel_scales — so it was the most natural home for this argument, and leaving it out would have left the module half-fixed. Default 0.01 reproduces the old -2.0 ladder exactly (np.log10(0.01) is exactly -2.0, same check as above).

3. The sigma_min docstring entry moved to match its position in the signature.

Full suite green: 1016 passed. Merging.

On our side the default stays 1e-4, so nothing changes for users until the examples pass the new argument — I'm picking that up as a workspace sweep, so you don't need to touch the workspace here. Thanks again, this was a real bug.

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