Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-24737

Implement GAaP: Gaussian Aperture and PSF photometry

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Story Points:
      32
    • Epic Link:
    • 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.

        Attachments

          Issue Links

            Activity

            Hide
            yusra Yusra AlSayyad added a comment -

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

            Show
            yusra Yusra AlSayyad added a comment - This ticket looks more epic-sized than story-sized. Split off a smaller first step if possible 
            Hide
            kannawad 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.

            Show
            kannawad 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.
            Hide
            kannawad 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. 

            Show
            kannawad 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
            kannawad 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. 

            Show
            kannawad 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. 
            Hide
            kannawad 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

            Show
            kannawad 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
            jbosch 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
            jbosch 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?
            Hide
            kannawad 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.

            Show
            kannawad 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.
            Hide
            kannawad 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`. 

            Show
            kannawad 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`. 
            Hide
            yusra 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.

            Show
            yusra 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
            kannawad 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.

            Show
            kannawad 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 .

              People

              Assignee:
              kannawad Arun Kannawadi
              Reporter:
              kannawad Arun Kannawadi
              Reviewers:
              Jim Bosch
              Watchers:
              Arun Kannawadi, Jim Bosch, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.