non-standard pixel sizes for distortion #669

JarronL · 2023-05-12T20:23:01Z

Previously, distortion code assumed that webbpsf detector pixels and pysiaf apertures had similar pixel scales. However, it is possible to specify any non-standard pixel size (e.g., inst.pixelscale = whatever) when generating a PSF. This fix correctly applies the pysiaf distortion model for any arbitrary pixel size.

Previously, distortion code assumed that webbpsf and pysiaf apertures had similar pixel sizes. However, it is possible to request any non-standard pixel size when generated a PSF. This fix correctly applies distortion for any arbitrary pixel size.

Need to apply the same pixel sampling along the x- and y- axis, otherwise test_apply_distortion_pixel_scale() fails. May need to update test?

codecov · 2023-05-12T21:59:39Z

Codecov Report

Patch coverage: 71.42% and project coverage change: -0.79% ⚠️

Comparison is base (427b93d) 55.97% compared to head (b4fdee2) 55.18%.

❗ Current head b4fdee2 differs from pull request most recent head 5685897. Consider uploading reports for the commit 5685897 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #669      +/-   ##
===========================================
- Coverage    55.97%   55.18%   -0.79%     
===========================================
  Files           15       14       -1     
  Lines         6280     6123     -157     
===========================================
- Hits          3515     3379     -136     
+ Misses        2765     2744      -21

Files Changed	Coverage Δ
webbpsf/distortion.py	`80.64% <71.42%> (+3.85%)`	⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mperrin · 2023-05-13T17:28:48Z

I'm trying to follow the logic behind this change - can you give me an example for some calculation or setup that doesn't work currently but does work properly with this change?

mperrin

Couple questions & comments after a first quick skim.

mperrin · 2023-05-13T17:18:00Z

webbpsf/distortion.py

@@ -63,7 +63,7 @@ def distort_image(hdulist_or_filename, ext=0, to_frame='sci', fill_value=0,
        Value used to fill in any blank space by the skewed PSF. Default = 0.
        If set to None, values outside the domain are extrapolated.
    to_frame : str
-        Type of input coordinates. 
+        Requested of output coordinate frame. 


Minor: Clarify/rephrase. "Requested of" doesn't make sense...

Right. Accidentally removed "type"

mperrin · 2023-05-13T17:22:54Z

webbpsf/distortion.py

+        # Choose conversion function
+        func_convert = getattr(aper, f'sci_to_{to_frame}')
+        xnew_cen, ynew_cen = func_convert(xsci_cen, ysci_cen)


Can you explain the motivation behind this change? It appears to me that this code is just replicating what the aper.convert function already does: check for a null transformation and if so return the input x,y; otherwise look up the correct X_to_Y function and then call it.

Ah, indeed. I had looked at the pysiaf convert function a long time ago and (perhaps erroneously) recall it making additional unnecessary transformations, so have always written my own set of if statements to minimize the number of calls. But looks like the logic now is much improved.

mperrin · 2023-05-13T17:24:57Z

webbpsf/distortion.py

+    if to_frame=='idl':
+        xnew_idl, ynew_idl = (xnew, ynew)
+    else:
+        # Choose conversion function
+        func_convert = getattr(aper, f'{to_frame}_to_idl')
+        xnew_idl, ynew_idl = func_convert(xnew, ynew)


Same comment as above; this appears redundant with what aper.convert already does?

Reverting to aper.convert.

JarronL · 2023-05-16T17:59:27Z

I'm trying to follow the logic behind this change - can you give me an example for some calculation or setup that doesn't work currently but does work properly with this change?

A colleague wanted to generate a MIRI PSF with a non-standard pixel size in order to match to the NIRCam PSF. So, by setting miri.pixelscale=0.061 (for instance), the distortion calculation would not correctly generate the new coordinates to perform the regular grid transformation onto. One symptom was that the total sum of a distorted PSF would be something like 30% of the sum from an undistorted PSF (ie., undistorted PSF is close to 1.0, while distorted PSF sum was 0.3).

I realized that when I originally authored this code, I made an assumptions about pixels scales and the meaning of oversample that were only applicable when the PSF pixel scale equals the SIAF pixel scale.

The updated code now generalizes the pixel scales such that I can still use SIAF to perform various coordinate transformations, but also making sure that I'm preserving the intended pixel scale when calculating the new set of coordinates.

mperrin · 2023-05-17T12:25:41Z

webbpsf/distortion.py

+        osamp_x = aper.XSciScale / pixelscale
+        osamp_y = aper.YSciScale / pixelscale
+        osamp = (osamp_x + osamp_y) / 2
+        xnew = xarr / osamp + xnew_cen
+        ynew = yarr / osamp + ynew_cen


Can you explain the logic here for averaging osamp? This computes first a distinct oversampling factor in each of X and Y, but then computes xnew and ynew using the average oversampling factor. Wouldn't it be more precise to calculate using the distinct oversampling factors per axis, like xnew = xarr / osamp_x + xnew_cen?

I originally tried doing it per-axis, but test_apply_distortion_pixel_scale() would fail. I did not have time to look into why the test was failing, as I didn't quite understand the logic behind the test. Reverting to a single oversampling applied across both axes fixed the issue. Need to investigate further.

Huh, yeah, now that I look at that test... I do not understand the logic either. 🤣

mperrin

I can replicate the bug you describe, and confirm this fixes it. I added a unit test that demonstrates/verifies that fix.

I do have the one question about oversampling per axis versus averaged; I'm honestly not sure which is the more precise/rigorous way to treat that, and would need to think through the math to double check. Apart from that clarification, this is ready to go.

mperrin · 2023-06-01T17:10:49Z

@JarronL FYI I tried to look at the test test_apply_distortion_pixel_scale and I can't really figure out the logic of that test and how it is supposed to work. It certainly is not doing what the docstring comment says it should do.

I am inclined to say just to delete that test, and you can edit this PR back to what you tried first, to treat the X and Y scales separately/

But really I should probably hand this off to @obi-wan76 to look at and think about, or to @Skyhawk172 who has been looking at the distortion code in the context of Roman.

Skyhawk172 · 2023-07-21T17:01:13Z

I don't understand test_apply_distortion_pixel_scale either. First, the test doesn't seem to do exactly what the docstring states; second, I'm not sure the "linear skew" assumption is perfectly valid.

With that said, I'd have to think hard about another more intuitive test... So I'm also inclined to reinstate Jarron's per-axis approach at the expense of removing this test (at least for the time being).

obi-wan76 · 2023-07-26T14:09:01Z

@JarronL what do you think? do you want to go back to doing it per-axis and removing test_apply_distortion_pixel_scale() ? Unless you have found the reason why the test is failing.

JarronL · 2023-07-26T20:32:15Z

Yes, I'm onboard with removing test_apply_distortion_pixel_scale() and reverting to per-axis:

        osamp_x = aper.XSciScale / pixelscale
        osamp_y = aper.YSciScale / pixelscale
        xnew = xarr / osamp_x + xnew_cen
        ynew = yarr / osamp_y + ynew_cen

JarronL · 2023-07-26T20:33:24Z

Would you like me to perform the change in this branch?

obi-wan76 · 2023-07-26T20:43:02Z

Yes, that would be great! Thanks!

test_apply_distortion_pixel_scale() now follows what its description Also updated test_apply_distortion_skew() so it actually creates a downsampled version of the data (previously was just creating a copy of the oversampled data).

JarronL requested a review from mperrin May 12, 2023 20:26

JarronL added 2 commits May 12, 2023 13:59

xidl_cen and yidl_cen need to be defined earlier

64348ab

fix test_apply_distortion_pixel_scale() failure

005a60f

Need to apply the same pixel sampling along the x- and y- axis, otherwise test_apply_distortion_pixel_scale() fails. May need to update test?

JarronL added the bug Something isn't working label May 12, 2023

mperrin reviewed May 13, 2023

View reviewed changes

revert to aper.convert

31926e7

mperrin reviewed May 17, 2023

View reviewed changes

add test to verify fix of distortion with nonstandard pixelscale

e511073

mperrin approved these changes May 17, 2023

View reviewed changes

JarronL and others added 3 commits July 26, 2023 15:42

updated test_apply_distortion_pixel_scale

454f1c4

test_apply_distortion_pixel_scale() now follows what its description Also updated test_apply_distortion_skew() so it actually creates a downsampled version of the data (previously was just creating a copy of the oversampled data).

use per axis pixel sampling during distortion

b4fdee2

Merge branch 'develop' into distortion_update

5685897

BradleySappington merged commit 90dba3f into develop Jul 27, 2023
12 checks passed

BradleySappington deleted the distortion_update branch July 27, 2023 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

non-standard pixel sizes for distortion #669

non-standard pixel sizes for distortion #669

JarronL commented May 12, 2023 •

edited

Loading

codecov bot commented May 12, 2023 •

edited

Loading

mperrin commented May 13, 2023

mperrin left a comment

mperrin May 13, 2023

JarronL May 16, 2023

mperrin May 13, 2023

JarronL May 16, 2023

mperrin May 13, 2023

JarronL May 16, 2023

JarronL commented May 16, 2023

mperrin May 17, 2023

JarronL May 17, 2023

mperrin May 18, 2023

mperrin left a comment

mperrin commented Jun 1, 2023

Skyhawk172 commented Jul 21, 2023

obi-wan76 commented Jul 26, 2023

JarronL commented Jul 26, 2023

JarronL commented Jul 26, 2023

obi-wan76 commented Jul 26, 2023

non-standard pixel sizes for distortion #669

non-standard pixel sizes for distortion #669

Conversation

JarronL commented May 12, 2023 • edited Loading

codecov bot commented May 12, 2023 • edited Loading

Codecov Report

mperrin commented May 13, 2023

mperrin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JarronL commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mperrin left a comment

Choose a reason for hiding this comment

mperrin commented Jun 1, 2023

Skyhawk172 commented Jul 21, 2023

obi-wan76 commented Jul 26, 2023

JarronL commented Jul 26, 2023

JarronL commented Jul 26, 2023

obi-wan76 commented Jul 26, 2023

JarronL commented May 12, 2023 •

edited

Loading

codecov bot commented May 12, 2023 •

edited

Loading