# Implement GAaP: Gaussian Aperture and PSF photometry

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s: None
• Labels:
• Story Points:
32
• Team:
Data Release Production
• Urgent?:
No

#### Description

In addition to Kron and CModel photometry, this ticket proposes to implement GAaP photometry (Kuijken 2008). For extended objects (galaxies), GAaP is not designed to give accurate fluxes, but provides accurate and robust colors. This ticket is a master-ticket to contain multiple tickets related to this work.

#### Activity

No builds found.
Field Original Value New Value
Epic Link DM-23736 [ 431392 ]
 Link This issue contains DM-24691 [ DM-24691 ]
 Link This issue contains DM-24686 [ DM-24686 ]
 Epic Link DM-23736 [ 431392 ] DM-25320 [ 435710 ]
 Summary Implement GAaP: Gaussian Aperture and PSF photometry Add Weak Lensing Metrics to Regular DRP Processing
 Status To Do [ 10001 ] In Progress [ 3 ]
 Summary Add Weak Lensing Metrics to Regular DRP Processing Implement GAaP: Gaussian Aperture and PSF photometry
Hide

This ticket looks more epic-sized than story-sized. Split off a smaller first step if possible

Show
Yusra AlSayyad added a comment - This ticket looks more epic-sized than story-sized. Split off a smaller first step if possible
 Epic Link DM-25320 [ 435710 ] DM-26794 [ 439744 ]
 Link This issue contains DM-27407 [ DM-27407 ]
 Link This issue contains DM-27408 [ DM-27408 ]
 Link This issue contains DM-27482 [ DM-27482 ]
 Link This issue contains DM-27482 [ DM-27482 ]
 Link This issue contains DM-27407 [ DM-27407 ]
 Link This issue contains DM-27408 [ DM-27408 ]
 Story Points 32
Hide

The link to the actual PR (in lsst-dm org) is https://github.com/lsst-dm/meas_extensions_gaap/pull/1

The repository has a mix of C++ and Python files. The C++ files are based heavily off of meas_base/GaussianFlux, with minor modifications. The reason to replicate the code instead of modifying the meas_base repo is because I expect to make further changes and it is best to keep the base code untouched. The gaapFlux implemented in C++ measures the flux with a Gaussian aperture after accounting for the GaussianPsf. The Python layer takes care of Gaussianizing the PSF and passing the Gaussianized exposure to C++ for measurement.

Show
Arun Kannawadi added a comment - - edited The link to the actual PR (in lsst-dm org) is https://github.com/lsst-dm/meas_extensions_gaap/pull/1 The repository has a mix of C++ and Python files. The C++ files are based heavily off of meas_base/GaussianFlux , with minor modifications. The reason to replicate the code instead of modifying the meas_base repo is because I expect to make further changes and it is best to keep the base code untouched. The gaapFlux implemented in C++ measures the flux with a Gaussian aperture after accounting for the GaussianPsf. The Python layer takes care of Gaussianizing the PSF and passing the Gaussianized exposure to C++ for measurement.
 Status In Progress [ 3 ] In Review [ 10004 ]
 Status In Review [ 10004 ] In Progress [ 3 ]
 Link This issue relates to DM-28024 [ DM-28024 ]
 Epic Link DM-26794 [ 439744 ] DM-27954 [ 442727 ]
 Link This issue relates to DM-28740 [ DM-28740 ]
Hide

The PR is to pull the ticket branch into u/kannawad branch as opposed to master in order to create a clean(er) pull request and master has some relic from its initial stages which I am yet to clean up.

Show
Arun Kannawadi added a comment - The PR is to pull the ticket branch into u/kannawad branch as opposed to master in order to create a clean(er) pull request and master has some relic from its initial stages which I am yet to clean up.
Hide

I tried to get in the review request before Focus Friday, but the origin/master branch and my ticket branch have diverged so much that I decided to pull into a temporary u/kannawad branch before I get everything in order.

Show
Arun Kannawadi added a comment - I tried to get in the review request before Focus Friday, but the origin/master branch and my ticket branch have diverged so much that I decided to pull into a temporary u/kannawad branch before I get everything in order.
 Reviewers Jim Bosch, Yusra AlSayyad [ jbosch, yusra ] Status In Progress [ 3 ] In Review [ 10004 ]
Hide

Link to the PR where the base branch is master: https://github.com/lsst-dm/meas_extensions_gaap/pull/3

Show
Arun Kannawadi added a comment - Link to the PR where the base branch is master:  https://github.com/lsst-dm/meas_extensions_gaap/pull/3
Hide
Jim Bosch added a comment -

I'm done reviewing, and while I've left a ton of comments on the PR, overall I think this looks pretty good, and the volume of comments just reflects this being your first big PR and the fact that it's in a sort of crusty part of the codebase (measurement plugins), where our best examples are sort of fossilized relative to modern style.

I've looked through all of this pretty throughly, so one approach to splitting up with the review with Yusra AlSayyad might be for her to take the second pass, after you think you've addressed my comments?

Show
Jim Bosch added a comment - I'm done reviewing, and while I've left a ton of comments on the PR, overall I think this looks pretty good, and the volume of comments just reflects this being your first big PR and the fact that it's in a sort of crusty part of the codebase (measurement plugins), where our best examples are sort of fossilized relative to modern style. I've looked through all of this pretty throughly, so one approach to splitting up with the review with Yusra AlSayyad might be for her to take the second pass, after you think you've addressed my comments?
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide

That sounds good to me. I probably won't be pushing any new changes to the Github repo until Monday, so if Yusra likes to have a first pass, she can do that before Monday and after that, it's probably best waiting for me to address Jim's comments.

Show
Arun Kannawadi added a comment - That sounds good to me. I probably won't be pushing any new changes to the Github repo until Monday, so if Yusra likes to have a first pass, she can do that before Monday and after that, it's probably best waiting for me to address Jim's comments.
 Link This issue relates to DM-28925 [ DM-28925 ]
 Link This issue relates to DM-28955 [ DM-28955 ]
Hide

Along with DM-28955, I have modified the plugin so that the modelPsfMatch task is constructed once during the plugin initialization and the alardSigGauss parameter can be called during the run time. I am not creating the PR on DM-28955 at the moment, because the size parameter of the AL Basis can be chosen dynamically using the scaleByFwhm parameter (bad naming that kept it hidden to me all this while). However, it is a different choice from one that Konraad chose to implement in his GAaP code. I'm replicating his choices as closely as possible, but I'd like to investigate how well the PSF Gaussianization is if I simply set scaleByFwhm=True.

Show
Arun Kannawadi added a comment - Along with DM-28955 , I have modified the plugin so that the modelPsfMatch task is constructed once during the plugin initialization and the alardSigGauss parameter can be called during the run time. I am not creating the PR on DM-28955 at the moment, because the size parameter of the AL Basis can be chosen dynamically using the scaleByFwhm parameter (bad naming that kept it hidden to me all this while). However, it is a different choice from one that Konraad chose to implement in his GAaP code. I'm replicating his choices as closely as possible, but I'd like to investigate how well the PSF Gaussianization is if I simply set scaleByFwhm=True.
 Status Reviewed [ 10101 ] In Review [ 10004 ]
 Link This issue is blocked by DM-29256 [ DM-29256 ]
 Reviewers Jim Bosch, Yusra AlSayyad [ jbosch, yusra ] Jim Bosch [ jbosch ]
Hide

Posted some comments on github and removed myself from the reviewer list. Main complaint is that it doesn't build.

Show
Yusra AlSayyad added a comment - Posted some comments on github and removed myself from the reviewer list. Main complaint is that it doesn't build.
Hide

I've fixed the build issue and made sure it builds successfully in a cloned repo as well. I made one other commit during pair coding with Jim this Wednesday. I think I've also addressed all other minor comments. I'll merge it to master on Monday and leave any remaining comments for smaller tickets later. This is going to remain an lsst-dm repo for some more time, at least until I wrap up DM-28955.

Show
Arun Kannawadi added a comment - I've fixed the build issue and made sure it builds successfully in a cloned repo as well. I made one other commit during pair coding with Jim this Wednesday. I think I've also addressed all other minor comments. I'll merge it to master on Monday and leave any remaining comments for smaller tickets later. This is going to remain an lsst-dm repo for some more time, at least until I wrap up DM-28955 .
 Status In Review [ 10004 ] Reviewed [ 10101 ]
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]

#### People

Assignee:
Reporter:
Reviewers:
Jim Bosch
Watchers: