search

GalSim-developers / GalSim

The modular galaxy image simulation toolkit. Documentation:
http://galsim-developers.github.io/GalSim/
Other
224 stars 105 forks source link

roman PSF fix #1142

Closed ztq1996 closed 2 years ago

ztq1996 commented 2 years ago

Change the unit conversion when reading roman files in roman.roman_psfs._read_aberrations(SCA)

rmjarvis commented 2 years ago

By the way, the test failure has nothing to do with your changes. Apparently distutils.version.LooseVersion is now deprecated. Since we don't actually need it anymore, I just removed our use of it in some tests. So if you rebase to the current releases/2.3, that error should go away. (I also cherry-picked a couple other recent things like this that had been fixed on main.)

ztq1996 commented 2 years ago

I have added a unit test that assert that the absolute values of the differences between the conjunction points are much less than or equal to the variation within the entire FOV. I removed some conjunction pairs compared to the original choices, because I think adjacent chips that has an offset (for example SCA15 and SCA12) should not have perfect continuity because they are not interpolated from the similar reference positions. The new sets of conjunction points looks like this image The test will pass for |Z_DIFF|< 0.05(Z_MAX - Z_MIN), although for the conjunction that are a little far away (for example SCA3 and SCA7), I have to set the condition to |Z_DIFF|< 0.10(Z_MAX - Z_MIN).

rmandelb commented 2 years ago

sounds promising; have you checked that e.g. an SCA orientation flip or units confusion in the interpolation will cause this test to fail?

ztq1996 commented 2 years ago

Yes, either the unit confusion or the orientation flip will fail the test.

rmandelb commented 2 years ago

Hi @ztq1996 - do you want to discuss any of my suggestions further? I think most are pretty minor, and it would be good to get this bug fix merged.

ztq1996 commented 2 years ago

Hi @rmandelb! Thanks for proposing these changes, I have accepted and made corresponding changes.

rmandelb commented 2 years ago

@rmjarvis - is the idea that we should merge this PR into releases/2.3 as a bug fix, but also get these changes onto master?

rmjarvis commented 2 years ago

Yep. I'll release the bugfix version and cherry-pick this onto main. Thanks for this fix Tianqing!

rmjarvis commented 2 years ago

v2.3.4 has been released.