# Fix position of psf computation for base_SdssShape_psf

## Details

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

## Description

There is an error in the psf computation of base_SdssShape_psf in meas_base's src/SdssShape.cc in that it does not provide the position of the source, so is just getting the measurement at the position returned by afw's getAveragePosition() in src/detection/Psf.cc for all sources. Please fix.

## Attachments

1. after-v1228-trace_-psfMagHist.png
81 kB
2. after-v1228-trace_-sky-stars.png
73 kB
3. before-v1228-trace_-psfMagHist.png
81 kB
4. before-v1228-trace_-sky-stars.png
73 kB

## Activity

Hide
Lauren MacArthur added a comment -

John Swinbank, could you have a look? I've attached some before and after fix plots and have confirmed that the catalog is now computing the psf shapes at the source position rather than just the image average position for each source:

before:

 In [10]: srcBefore["base_SdssShape_psf_xx"] Out[10]:  array([ 4.35643166, 4.35643166, 4.35643166, ..., 4.35643166, 4.35643166, 4.35643166]) 

after (for same visit/ccd combo);

 In [12]: srcAfter["base_SdssShape_psf_xx"] Out[12]:  array([ 4.35188276, 4.35838516, 4.37397443, ..., 4.31416827, 4.31451534, 4.31364129]) 

Show
Lauren MacArthur added a comment - John Swinbank , could you have a look? I've attached some before and after fix plots and have confirmed that the catalog is now computing the psf shapes at the source position rather than just the image average position for each source: before: In [10]: srcBefore["base_SdssShape_psf_xx"] Out[10]: array([ 4.35643166, 4.35643166, 4.35643166, ..., 4.35643166, 4.35643166, 4.35643166]) after (for same visit/ccd combo); In [12]: srcAfter["base_SdssShape_psf_xx"] Out[12]: array([ 4.35188276, 4.35838516, 4.37397443, ..., 4.31416827, 4.31451534, 4.31364129]) Jenkins is happy: https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py2/19409/console
Hide
John Swinbank added a comment -

Plots look nice. Code change seems to be unambiguously the right thing. Well found.

But... (& please don't hate me for this) any chance of dropping in a test case to make sure this doesn't happen again? I've not looked at the SdssShape tests to see how straightforward it would be.

Show
John Swinbank added a comment - Plots look nice. Code change seems to be unambiguously the right thing. Well found. But... (& please don't hate me for this) any chance of dropping in a test case to make sure this doesn't happen again? I've not looked at the SdssShape tests to see how straightforward it would be.
Hide
Lauren MacArthur added a comment -

I don't hate you at all, and in fact already looked into it. It's not entirely straight-forward to test for this the way we're currently using testSdssShape.py to test the basic functionality as the test image built is almost as simplistic as it gets, only having one psf model for the entire image (so you'd get the same psf shape at any position of the image). I'll have a closer look to see if it can be adapted...

Show
Lauren MacArthur added a comment - I don't hate you at all, and in fact already looked into it. It's not entirely straight-forward to test for this the way we're currently using testSdssShape.py to test the basic functionality as the test image built is almost as simplistic as it gets, only having one psf model for the entire image (so you'd get the same psf shape at any position of the image). I'll have a closer look to see if it can be adapted...
Hide
Lauren MacArthur added a comment -

Ok, many thanks to Yusra AlSayyad for showing me how to create a variable psf model that could be applied to an exposure so that we could put in a unittest for this issue. The only hitch is that it requires use of meas_algorithms's PcaPsf, so the test can't live in meas_base. I've added tests/testSdssShapePsf.py to meas_algorithms. Below I show a printout for the measurements at 3 different source locations to ensure the psf is actually varying:

 source position: 50.1 49.8  psfTruth = (ixx=3.89667634765, iyy=3.89667634765, ixy=-0.0) sdssShape_psf = (ixx=3.89668177791, iyy=3.89668177791, ixy=-0.0) source position: -11.6 -1.7  psfTruth = (ixx=2.97828641118, iyy=2.97828641118, ixy=-0.0) sdssShape_psf = (ixx=2.978288526, iyy=2.978288526, ixy=-0.0) source position: 149.9 50.3  psfTruth = (ixx=8.99930898191, iyy=8.99930898191, ixy=0.0) sdssShape_psf = (ixx=8.99930656829, iyy=8.99930656829, ixy=0.0) 

I also added a note to tests/testSdssShape.py in meas_base noting this.

Jenkins is down, but the test passes on my local build. Feel free to have a look when you have time and I'll let you know when I get the Jenkins run going.

Show
Lauren MacArthur added a comment - Ok, many thanks to Yusra AlSayyad for showing me how to create a variable psf model that could be applied to an exposure so that we could put in a unittest for this issue. The only hitch is that it requires use of meas_algorithms 's PcaPsf , so the test can't live in meas_base . I've added tests/testSdssShapePsf.py to meas_algorithms . Below I show a printout for the measurements at 3 different source locations to ensure the psf is actually varying: source position: 50.1 49.8 psfTruth = (ixx=3.89667634765, iyy=3.89667634765, ixy=-0.0) sdssShape_psf = (ixx=3.89668177791, iyy=3.89668177791, ixy=-0.0) source position: -11.6 -1.7 psfTruth = (ixx=2.97828641118, iyy=2.97828641118, ixy=-0.0) sdssShape_psf = (ixx=2.978288526, iyy=2.978288526, ixy=-0.0) source position: 149.9 50.3 psfTruth = (ixx=8.99930898191, iyy=8.99930898191, ixy=0.0) sdssShape_psf = (ixx=8.99930656829, iyy=8.99930656829, ixy=0.0) I also added a note to tests/testSdssShape.py in meas_base noting this. Jenkins is down, but the test passes on my local build. Feel free to have a look when you have time and I'll let you know when I get the Jenkins run going.
Hide
Lauren MacArthur added a comment -
Show
Lauren MacArthur added a comment - Jenkins is happy: https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py2/19428/console
Hide
John Swinbank added a comment -

Added a fairly trivial comment to the PR on meas_algorithms. Otherwise, ok to merge.

By the way, the current standards say you should create your own PRs for review. See the discussion here. Not a big deal, obviously!

Happy holidays.

Show
John Swinbank added a comment - Added a fairly trivial comment to the PR on meas_algorithms. Otherwise, ok to merge. By the way, the current standards say you should create your own PRs for review. See the discussion here . Not a big deal, obviously! Happy holidays.
Hide
Lauren MacArthur added a comment -

Thanks John Swinbank. I added the extra checks, reran Jenkins, and merged to master.

Show
Lauren MacArthur added a comment - Thanks John Swinbank . I added the extra checks, reran Jenkins, and merged to master.

## People

• Assignee:
Lauren MacArthur
Reporter:
Lauren MacArthur
Reviewers:
John Swinbank
Watchers:
John Swinbank, Lauren MacArthur