Details

    • Type: Story
    • Status: Done
    • Priority: Major
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_extensions_psfex
    • 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.

        Issue Links

          Activity

          Hide
          price Paul Price added a comment -

          This is a cherry-pick of HSC-1412.

          Show
          price Paul Price added a comment - This is a cherry-pick of HSC-1412 .
          Hide
          price 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(-)
          

          Show
          price 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
          swinbank 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
          swinbank 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
          price Paul Price added a comment -

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

          Show
          price Paul Price added a comment - It's failing a unit test. Taking this back.
          Hide
          price 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
          price 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
          swinbank 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
          swinbank 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
          price 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.
          

          Show
          price 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
          swinbank 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
          swinbank 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
          price 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
          price 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:
              price Paul Price
              Reporter:
              rearmstr Bob Armstrong
              Reviewers:
              John Swinbank
              Watchers:
              Bob Armstrong, John Swinbank, Paul Price
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile