# Fix oversampling settings in psfex

## Details

• Type: Story
• Status: Done
• Priority: Major
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Templates:
• Story Points:
1
• Sprint:
DRP F16-2
• Team:
Data Release Production

## Description

The current settings in psfex will only turn on oversampling only if the seeing is < 0.5", even if you have configured it do oversampling. This needs to be changed so that everything is determined by the config parameters.

We have also seen on HSC data that oversampling in general does not work well in psfex. We need to change the current configuration which does 2x oversampling to just use the native pixel scale.

## Activity

Hide
Paul Price added a comment -

This is a cherry-pick of HSC-1412.

Show
Paul Price added a comment - This is a cherry-pick of HSC-1412 .
Hide
Paul Price added a comment -

John Swinbank, would you mind having a look at this pleaes? These two simple commits have already been reviewed for HSC, but I got a merge conflict as part of the cherry-pick due to refactoring on the LSST side so another pair of eyes would be good. It's going through Jenkins now.

 price@price-laptop:~/LSST/meas/extensions/psfex (tickets/DM-6982=) $git sub commit aed6e758cdc822ccd965a9e6e79b32305cbc1c62 Author: Bob Armstrong  Date: Wed Jul 20 10:54:25 2016 -0400    Change psfex basis to PIXEL    The previous psfex basis was set to PIXEL_AUTO which only allows  the basis to be oversampled if the seeing < 0.5". This change  forces psfex to always use the settings given in the config to  determine the amount of oversampling.    config/default-lsst.psfex | 2 +-  1 file changed, 1 insertion(+), 1 deletion(-)   commit 6b37fb95e54f59c1e29d0e67b9f1c3650bee99c2 Author: Bob Armstrong  Date: Wed Jul 20 11:00:28 2016 -0400    Change the default resolution of the PSF model to the native pixel size.    python/lsst/meas/extensions/psfex/psfexPsfDeterminer.py | 4 ++--  1 file changed, 2 insertions(+), 2 deletions(-)  Show Paul Price added a comment - John Swinbank , would you mind having a look at this pleaes? These two simple commits have already been reviewed for HSC, but I got a merge conflict as part of the cherry-pick due to refactoring on the LSST side so another pair of eyes would be good. It's going through Jenkins now. price@price-laptop:~/LSST/meas/extensions/psfex (tickets/DM-6982=)$ git sub commit aed6e758cdc822ccd965a9e6e79b32305cbc1c62 Author: Bob Armstrong <rearmstr@gmail.com> Date: Wed Jul 20 10:54:25 2016 -0400   Change psfex basis to PIXEL The previous psfex basis was set to PIXEL_AUTO which only allows the basis to be oversampled if the seeing < 0.5". This change forces psfex to always use the settings given in the config to determine the amount of oversampling.   config/default-lsst.psfex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)   commit 6b37fb95e54f59c1e29d0e67b9f1c3650bee99c2 Author: Bob Armstrong <rearmstr@gmail.com> Date: Wed Jul 20 11:00:28 2016 -0400   Change the default resolution of the PSF model to the native pixel size.   python/lsst/meas/extensions/psfex/psfexPsfDeterminer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Hide
John Swinbank added a comment -

I eyeballed the code changes, which look innocent enough.

However, I note that Jenkins is unhappy. I further note that the same test is run on HSC, including the same limiting value (4.6). I assume it passes on HSC, so the fact that this now fails on LSST makes me suspect that something isn't working properly.

Could you give that another check, please?

Show
John Swinbank added a comment - I eyeballed the code changes, which look innocent enough. However, I note that Jenkins is unhappy . I further note that the same test is run on HSC, including the same limiting value (4.6). I assume it passes on HSC, so the fact that this now fails on LSST makes me suspect that something isn't working properly. Could you give that another check, please?
Hide
Paul Price added a comment -

It's failing a unit test. Taking this back.

Show
Paul Price added a comment - It's failing a unit test. Taking this back.
Hide
Paul Price added a comment -

The unit test also fails on the HSC side (but unit test failures there aren't fatal to the build, so it seems no-one noticed).

Show
Paul Price added a comment - The unit test also fails on the HSC side (but unit test failures there aren't fatal to the build, so it seems no-one noticed).
Hide
John Swinbank added a comment - - edited

Assuming that the test fails in an expected way – ie, you and Bob Armstrong are happy that you can explain the failure and can modify the test case appropriately – I have no problem with this being merged. If you need further changes to the code, rather than just adjust the numbers in the test, by all means send it back to me for another look.

Show
John Swinbank added a comment - - edited Assuming that the test fails in an expected way – ie, you and Bob Armstrong are happy that you can explain the failure and can modify the test case appropriately – I have no problem with this being merged. If you need further changes to the code, rather than just adjust the numbers in the test, by all means send it back to me for another look.
Hide
Paul Price added a comment -

I found the cause of the test failure — oversampling hadn't been disabled in the test.

 pprice@tiger-sumire:~/LSST/meas/extensions/psfex (tickets/DM-6982=) $git --no-pager log -2 --reverse commit a8046b3778507c70327a79b4fc2d48a30ad7cfc8 Author: Paul Price  Date: Tue Jul 26 16:41:00 2016 -0400    disable oversampling in test    We believe that oversampling doesn't work well. Indeed, with  oversampling turned off, we get better fits for the test image.   commit 164e2651a573d7314ca865f41d46b8f68f7cbcfb Author: Paul Price  Date: Tue Jul 26 16:41:14 2016 -0400    remove unused and confusing line in test    self.FWHM is unused, and its presence is confusing because there  is also sigma1 and sigma2 which are used to set the PSF, and they  have no relation to self.FWHM.  Show Paul Price added a comment - I found the cause of the test failure — oversampling hadn't been disabled in the test. pprice@tiger-sumire:~/LSST/meas/extensions/psfex (tickets/DM-6982=)$ git --no-pager log -2 --reverse commit a8046b3778507c70327a79b4fc2d48a30ad7cfc8 Author: Paul Price <price@astro.princeton.edu> Date: Tue Jul 26 16:41:00 2016 -0400   disable oversampling in test We believe that oversampling doesn't work well. Indeed, with oversampling turned off, we get better fits for the test image.   commit 164e2651a573d7314ca865f41d46b8f68f7cbcfb Author: Paul Price <price@astro.princeton.edu> Date: Tue Jul 26 16:41:14 2016 -0400   remove unused and confusing line in test self.FWHM is unused, and its presence is confusing because there is also sigma1 and sigma2 which are used to set the PSF, and they have no relation to self.FWHM.
Hide
John Swinbank added a comment -

Line 231 of testPsfexPsf.py is a comment which reads "Include oversampling in test". It immediately precedes the lines you've edited in a8046b3 to "disable oversampling in test". Please make these consistent!

I note that the test passes if you simply use the default kernelSize and samplingSize rather than overriding them in the test case. Is the override actually necessary? Can you just remove it (and the comment) for simplicity?

Show
John Swinbank added a comment - Line 231 of testPsfexPsf.py is a comment which reads "Include oversampling in test". It immediately precedes the lines you've edited in a8046b3 to "disable oversampling in test". Please make these consistent! I note that the test passes if you simply use the default kernelSize and samplingSize rather than overriding them in the test case. Is the override actually necessary? Can you just remove it (and the comment) for simplicity?
Hide
Paul Price added a comment -

I'm such a doofus. I remember thinking that I could just remove those lines, but must have gotten side-tracked. They're gone now, the test passes, and I've merged to master.

Show
Paul Price added a comment - I'm such a doofus. I remember thinking that I could just remove those lines, but must have gotten side-tracked. They're gone now, the test passes, and I've merged to master.

## People

• Assignee:
Paul Price
Reporter:
Bob Armstrong
Reviewers:
John Swinbank
Watchers:
Bob Armstrong, John Swinbank, Paul Price