ENH add Interpolant classes #56
Conversation
|
Hello Matthew, BTW: if you impose float32 variable init (cf. jnp.zero_like...) then the code crashes if I use a JAX config with float64 variables. |
|
I cannot access the notebook. However the code in this pr matches galsim to pretty high precision so I'm guessing there isn't a bug in the code here. |
|
I've opened the access, sorry. No doubt that is an argument miss fit |
|
Fixed here: https://colab.research.google.com/drive/1tk8vPSiZac5PytN639dParpam9pR695w?usp=sharing It was indeed the |
|
I'm glad to see that I have made a valuable x-check of the code, thanks having spent a bit to correct the nb. |
There was a problem hiding this comment.
This is a complex PR! As long as I can tell it's all correct, I carefully compared your implementation and constants with what is in Interpolants.cpp and Interpolants.h in Galsim. If there is anything I couldn't find or was unclear, I added a question below.
Of course, there's a chance I missed something so I also pinged @jecampagne for a review. But I think I would be ok approving once the questions below are clear (since all tests you added are passing and I think you are additional waiting for all InterpolatedImage tests in #60 to pass as well before merging this one)
|
OK @ismael-mendoza - this one is ready to merge. I am still working on the interpolated image itself but I don't foresee a ton of changes here. |
There was a problem hiding this comment.
Just had a small comment @beckermr
Is there any part of the code that significantly changed since my last review (or did you just rebased the branch)? Didn't notice anything but let me know otherwise
| ) | ||
|
|
||
| @staticmethod | ||
| def from_name(name, tol=None, gsparams=None): |
There was a problem hiding this comment.
why not wrap this docstring?
There was a problem hiding this comment.
No reason. The code base is not consistent on this, but I am leaving that to another PR.
|
I just rebased yes. |
This PR adds the interpolant classes from galsim.