Skip to content

fix: make stepk match galsim#221

Merged
beckermr merged 23 commits intomainfrom
stepk-fix
Apr 24, 2026
Merged

fix: make stepk match galsim#221
beckermr merged 23 commits intomainfrom
stepk-fix

Conversation

@beckermr
Copy link
Copy Markdown
Collaborator

This PR gets stepk for interpolated images to match galsim exactly. 🎉

xref: #189

@beckermr beckermr marked this pull request as draft April 23, 2026 18:58
Comment thread jax_galsim/interpolatedimage.py
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 23, 2026

Merging this PR will not alter performance

⚠️ 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.

✅ 36 untouched benchmarks


Comparing stepk-fix (2f1bdb4) with main (88d4aec)

Open in CodSpeed

Comment thread jax_galsim/interpolatedimage.py Outdated
Comment thread jax_galsim/interpolatedimage.py Outdated
@beckermr
Copy link
Copy Markdown
Collaborator Author

Test failures here are due to maxk not matching.

@beckermr beckermr marked this pull request as ready for review April 24, 2026 14:40
@beckermr beckermr requested a review from ismael-mendoza April 24, 2026 14:47
@beckermr
Copy link
Copy Markdown
Collaborator Author

Let's go ahead with this PR for now. I still want to look into maxk more, but this PR leaves the code in a pretty good working state.

@beckermr
Copy link
Copy Markdown
Collaborator Author

@ismael-mendoza this is ready for review!

@ismael-mendoza
Copy link
Copy Markdown
Collaborator

thanks! I can take a look today

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.

Just a few questions and some changes. If you think the first two are actually a bug, not sure if these should go into a different PR...?

Comment thread jax_galsim/interpolatedimage.py Outdated
Comment thread jax_galsim/interpolatedimage.py Outdated
Comment thread jax_galsim/interpolatedimage.py
Comment thread jax_galsim/interpolatedimage.py Outdated
Comment thread tests/jax/test_interpolatedimage_utils.py
@beckermr
Copy link
Copy Markdown
Collaborator Author

Let's see if removing this unused code works OK. I think it might and I do not recall why or what I was trying to do before.

@beckermr
Copy link
Copy Markdown
Collaborator Author

OK @ismael-mendoza! Thank you for the very helpful code review! This is ready for another look.

@beckermr beckermr requested a review from ismael-mendoza April 24, 2026 18:15
Comment thread jax_galsim/interpolatedimage.py Outdated
@beckermr
Copy link
Copy Markdown
Collaborator Author

@ismael-mendoza I recalled the general reason for the two classes here and added some notes on it.

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.

Thanks LGTM!

@beckermr beckermr merged commit c8714ed into main Apr 24, 2026
9 checks passed
@beckermr beckermr deleted the stepk-fix branch April 24, 2026 22:11
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