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