-
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
Implementation of k-space convolution #44
Conversation
@ismael-mendoza could you check all the add/sub/mul things in image.py? |
Somethings that are missing so far:
|
30f1b2f
to
5360fff
Compare
just rebased with the
|
Awesome :-D |
Well, so, for some reason there appears to be an offset in the fft drawing method, also I still am not sure whether the rotations work correctly, this should be tested |
So, I did 2 additional things:
|
One quick note before leaving for the airport, the reason the Fourier transform is behaving in a funny way is that I found a bug yesterday here: JAX-GalSim/jax_galsim/gsobject.py Line 812 in 477c2ce
|
ok, so with the latest fixes, the drawImage (method='fft') wotks again, but I don't think it works as well as what we had the other day (which was actually buggy as it was using arrays of the wrong size, but thanks to JAX not checking for bounds, ended up working nevertheless). We currently have only one test that fails, but it's a pretty important one which tests whether shear, rotations, and shifts work properly. For sure it should get sorted before we merge. That's probably as far as I can go on this project for now unfortunately :-( |
@EiffL - thanks for all your work on this. Could you elaborate a bit on why you think the |
If you plot residuals vs galsim there are some ugly aliasing patterns |
Ok :-) I think I fixed all the main problems. All the tests pass now, and I also added a few ones to do some basic checks against the reference GalSim in def func(galsim):
gal = galsim.Gaussian(flux=1, sigma=1).shear(e1=0.1, e2=0.)
pixel = galsim.Pixel(scale=0.2, flux=1.0)
conv = galsim.Convolve([gal, pixel])
return conv.drawImage(
nx=32, ny=32, scale=0.2, method="sb").array
np.testing.assert_array_almost_equal(
func(ref_galsim), func(jax_galsim),
5,
err_msg="Using GSObject Convolve([psf,pixel]) disagrees with expected result") So, anyway, I think this PR is fully ready for review @ismael-mendoza |
Amazing, thanks @EiffL ! Will review soon, very interested in hearing about the fix at some point. |
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.
It looks very good! All the tests are similarly passing for me. Although I do have some questions below but not sure if it stop merging.
@jecampagne feel free to chime in if you are able to since a lot of the code was borrowed from you.
One last outstanding thing if we should add the draw methods for the Exponential here or in a different PR (EDIT: looking at wrong file)
tests/GalSim
Outdated
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.
@EiffL did you update something in Galsim?
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.
Hi @ismael-mendoza , yes I updated to a specific galsim branch where we can push updates for the tests:
https://github.com/GalSim-developers/GalSim/tree/fix_tests
Hi @EiffL @ismael-mendoza you did an amazing progress and if I've contributed to this development then I am pleased having not waste your time. |
Thanks @jecampagne :-) I've mostly copy/pasted your code over and did a bunch of reformatting, I credited you in the corresponding commits so that your code contributions will be tracked in the project history :-) |
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.
LGTM! I think it's ready to merge :D
Looks ok me too. |
Wouhou! Merging :-D |
This PR adds the ability to perform Fourier-based PSF convolutions. Most of this code comes from @jecampagne.