Skip to content

feat: investigate maxk differences and add galsim algorithm for maxk#224

Merged
beckermr merged 10 commits intomainfrom
fix-maxk
Apr 30, 2026
Merged

feat: investigate maxk differences and add galsim algorithm for maxk#224
beckermr merged 10 commits intomainfrom
fix-maxk

Conversation

@beckermr
Copy link
Copy Markdown
Collaborator

@beckermr beckermr commented Apr 27, 2026

I have developed a scan-based implementation of galsim's algorithm for maxk. This algorithm is ~10-20% slower than the one we use now. In most cases it does not make a difference which one we use, so I am opting for the faster one.

I also went through some of the changes in the test suite for maxk and adjusted the threshold for one test for Moffat profiles. I also enabled more of the Moffat tests to be run.

closes #225
closes #189

For associated changes to the test suite see: GalSim-developers/GalSim-for-JAX-GalSim-Testing#4

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 27, 2026

Merging this PR will degrade performance by 32.22%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 7 improved benchmarks
❌ 1 regressed benchmark
✅ 28 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
WallTime test_benchmarks_lanczos_interp[xval-no_conserve_dc-run] 114.1 µs 72.8 µs +56.76%
WallTime test_benchmarks_lanczos_interp[xval-conserve_dc-run] 129.5 µs 81.2 µs +59.61%
WallTime test_benchmarks_lanczos_interp[kval-no_conserve_dc-run] 56.4 µs 41.3 µs +36.75%
WallTime test_benchmarks_lanczos_interp[kval-conserve_dc-run] 55.3 µs 40.6 µs +36.33%
WallTime test_benchmark_moffat_init[run] 97.3 µs 67.7 µs +43.65%
WallTime test_benchmark_spergel_calcfluxrad[run] 215.2 µs 165.4 µs +30.07%
WallTime test_benchmark_invert_ab_noraise[run] 144.7 µs 104.2 µs +38.85%
WallTime test_benchmark_spergel_conv[run] 156.4 ms 230.7 ms -32.22%

Comparing fix-maxk (ffcef75) with main (e3ff71f)

Open in CodSpeed

@beckermr beckermr marked this pull request as draft April 28, 2026 09:04
@beckermr beckermr changed the title test: remove setting maxk since this now works too fix: use galsim algorithm for maxk Apr 28, 2026
Comment thread jax_galsim/interpolatedimage.py Outdated
@beckermr beckermr changed the title fix: use galsim algorithm for maxk feat: add galsim algorithm for maxk Apr 29, 2026
@beckermr beckermr marked this pull request as ready for review April 29, 2026 18:24
@beckermr
Copy link
Copy Markdown
Collaborator Author

OK This Pr is ready to go. It documents a bunch of work I've done to investigate how we set maxk for interpolated images. TL;DR; what we do now matches galsim exactly in most situations. We can write an algorithm that does exactly what galsim does, but it is more expensive and so doesn't seem worth the trouble. I have left the new algorithm commented out in the code. We can switch to it in the future if we feel it is important.

@beckermr beckermr changed the title feat: add galsim algorithm for maxk feat: investogate maxk differences and add galsim algorithm for maxk Apr 29, 2026
@beckermr beckermr changed the title feat: investogate maxk differences and add galsim algorithm for maxk feat: investigate maxk differences and add galsim algorithm for maxk Apr 29, 2026
@beckermr
Copy link
Copy Markdown
Collaborator Author

@ismael-mendoza this one is ready to go!

@beckermr beckermr requested a review from ismael-mendoza April 29, 2026 18:36
Copy link
Copy Markdown
Collaborator

@ismael-mendoza ismael-mendoza left a comment

Choose a reason for hiding this comment

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

LGTM!

@beckermr beckermr merged commit 58d1a0e into main Apr 30, 2026
9 checks passed
@beckermr beckermr deleted the fix-maxk branch April 30, 2026 11:29
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.

do not set maxk in tests stepk and maxk for interpolated_images are computed different than galsim

2 participants