-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH add InterpolatedImage #60
Conversation
This PR is blocked on figuring out the differences in this galsim issue: GalSim-developers/GalSim#1248 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blacken
OK. I figured out the issue here. You need the k-space wrapping and you need to set maxk and stepk to be the same values. Those values for interpolated images in jax-galsim differ a little from galsim and cause rendering differences under convolutions. Once you do this, metacal agrees to better than 1e-7 in absolute tolerance. |
Alright @ismael-mendoza. This PR is a monster, but is ready for review. If you have suggestions on how to break it into smaller chunks, I'd be happy to do that. |
Thanks! will start taking a look later today and let you know if I have suggestions on how to split it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK @beckermr here is my initial review, still need to review a couple of parts of interpolatedimage.py
namely the functions that you had to borrow from C++ code in Galsim
OK @ismael-mendoza. Back to you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK almost done, I just don't understand two functions below
OK @ismael-mendoza ready for one last look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the comments @beckermr ! Now it makes sense, just one small comment below and I think it's ready to merge
This PR adds interpolated images.
Closes #54