DRAFT (do not merge): Field Varying PSF #208
Conversation
Preliminary implementation of GridFieldVaryingPSF for convolving FOVs with field-varying PSFs using interpolation. The implementation might look somewhat convoluted, and it does not rely on many existing Numpy and SciPy methods, but this is done purposely to enable Numba compilation and parallelization. Missing: - Defining the structure of the input PSF grid; - Reading PSF grid coordinates and perform correct positioning of the PSF; - Kernel normalization; - Expand for multiple wavelengths.
Release candidate of PSF effect using a grid of PSFs that are interpolated across the field. Working example, but further testing required. No multiple-wavelength compatibility yet. - Compatibility with 3.5 lost (uses f-strings). Though these seem to be used in other places, so it probably was lost before. - Some docstring incomplete, mostly due to obfuscated attributes or missing or not well-defined type hints.
Raising exceptions for pixels out of bounds during numba routines wasn't working, so it was removed and made it compulsory that the coordinates to be computed lie withing the PSF grid.
There was a problem hiding this comment.
Looking good! I'm mainly confused about the 1.5 (see inline comment). I'll comment more in #193
scopesim/effects/psfs.py
Outdated
| dx=hdr["CDELT1"] * unit_factor_1*1.5, | ||
| dy=hdr["CDELT2"] * unit_factor_2*1.5, |
There was a problem hiding this comment.
Why is there a 1.5 here?
There was a problem hiding this comment.
Thank you for finding this. I had been doing some tests and this was left behind by mistake. That 1.5 factor should be removed.
| return self.n, | ||
|
|
||
|
|
||
| class GridCoordinates: |
There was a problem hiding this comment.
It is understandable why you added these classes, as I've found it quite annoying to do the calculations by hand.
However, I'm thinking that it might be better to directly use astropy wcs as that is what we will have to do at some point anyway once we start to model the projections better.
Nevertheless, I'm fine with using your implementation for now, as it is more than sufficient for our current goals.
There was a problem hiding this comment.
I am not very familiar with WCS and more so with the astropy implementation, but I'll take a look when I have the chance. I also believe that my implementation works for now, but if WCS is well suited for this then it would be interesting to use it to avoid more custom code and increase the probability of errors.
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) | ||
|
|
||
| self.meta["z_order"] = [262, 662] # [JA] TODO: Is this ok? |
There was a problem hiding this comment.
This z_order is probably fine. The hundreds-digit has some semantic meaning (which is not properly documented...) and the other are the order.
|
FYI, yesterday we merged #276, which corrects the off-by-one error that caused some pain, perhaps especially for PSFs. Getting all the edge cases out was a huge task, hopefully we got them all. I'm now writing some general PSF tests. E.g. creating a simulation of a single star with an analytical PSF, and then subsequently using that as a numerical PSF in a subsequent simulation. The results are now identical in shape, as expected. Then I'd like to take it from there and create more complex consistency tests, starting with a field varying PSF that does not vary, and see whether that gives an identical shape as well, etc. The idea is that this would provide a testing framework for the different implementations like this one. |
|
Replaced with #403 because I still want to continue this |
This is a PR to make it easy to see and discuss the code from @joao-aveiro in #193