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

Adapt CFHT astrometry test for DECam COSMOS field validation

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Adapt the CFHT astrometry validation test to the DECam reprocessing effort.

      Will focus on the repeat observations of the COSMOS field. Goal is to just do a simple two-observation comparison. Doing a full test of all of the observations will be a later story.

        Attachments

          Issue Links

            Activity

            Hide
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

            Test done and successful. Need to find a place for the `valid_cosmos.py` code to live.

            Show
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Test done and successful. Need to find a place for the `valid_cosmos.py` code to live.
            Hide
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

            moved things all into `http://github.com/lsst/validate_drp`

            Will request review once main `validate_drp` for CFHT is approved.

            Show
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - moved things all into ` http://github.com/lsst/validate_drp ` Will request review once main `validate_drp` for CFHT is approved.
            Hide
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

            Moderately quick review

            Depends on tickets/DM-4814 to get the data into `validation_data_decam`, so do that one first.

            Takes perhaps 15 minutes to run the DECam demo.

            Runs same analysis as the CFHT analysis from DM-4706, DM-4709. There's been some refactoring for this generalization to support two survey+cameras.

            Re-organizing the code to match standard DM packages is deferred to DM-4827.

            Show
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Moderately quick review Depends on tickets/ DM-4814 to get the data into `validation_data_decam`, so do that one first. Takes perhaps 15 minutes to run the DECam demo. Runs same analysis as the CFHT analysis from DM-4706 , DM-4709 . There's been some refactoring for this generalization to support two survey+cameras. Re-organizing the code to match standard DM packages is deferred to DM-4827 .
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            I haven't run the demo yet due to the git-lfs problem. But so far I found some commit history can be rewritten for clarity. For example,

            • All whitespace changes can be squashed into one
            • The filename run.list got changed to run_cfht.list and then runCfht.list. It doesn't need the middle step.
            • Since the DECam analysis script is a new addition in this ticket, it might as well be added after the generalized check_astrometry.py. Right now a lot got added there and then got deleted. Its name got changed after it was first added too.

            Similar ideas for changes that were one way and then changed to another within this ticket, as our policy encourages us to "make the commit history look like the code was written correctly the first time." (https://confluence.lsstcorp.org/display/LDMDG/Git+Commit+Best+Practices)

            Show
            hchiang2 Hsin-Fang Chiang added a comment - I haven't run the demo yet due to the git-lfs problem. But so far I found some commit history can be rewritten for clarity. For example, All whitespace changes can be squashed into one The filename run.list got changed to run_cfht.list and then runCfht.list. It doesn't need the middle step. Since the DECam analysis script is a new addition in this ticket, it might as well be added after the generalized check_astrometry.py. Right now a lot got added there and then got deleted. Its name got changed after it was first added too. Similar ideas for changes that were one way and then changed to another within this ticket, as our policy encourages us to "make the commit history look like the code was written correctly the first time." ( https://confluence.lsstcorp.org/display/LDMDG/Git+Commit+Best+Practices )
            Hide
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

            Yes. This is the commit history that I agree can be simplified!

            Show
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Yes. This is the commit history that I agree can be simplified!
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            I was able to run runDecamTest.sh and obtained validating outputs as follows:

            5019 sources in ccd: 10
            5283 sources in ccd: 11
            5192 sources in ccd: 12
            5395 sources in ccd: 13
            5171 sources in ccd: 14
            5831 sources in ccd: 15
            5726 sources in ccd: 16
            5209 sources in ccd: 17
            5428 sources in ccd: 18
            48254 total sources in reference visit.
            Visits : [{'filter': 'z', 'ccdnum': 10, 'visit': 176846}] 1104 matches found
            Visits : [{'filter': 'z', 'ccdnum': 11, 'visit': 176846}] 1198 matches found
            Visits : [{'filter': 'z', 'ccdnum': 12, 'visit': 176846}] 1211 matches found
            Visits : [{'filter': 'z', 'ccdnum': 13, 'visit': 176846}] 503 matches found
            Visits : [{'filter': 'z', 'ccdnum': 14, 'visit': 176846}] 1008 matches found
            Visits : [{'filter': 'z', 'ccdnum': 15, 'visit': 176846}] 1710 matches found
            Visits : [{'filter': 'z', 'ccdnum': 16, 'visit': 176846}] 1961 matches found
            Visits : [{'filter': 'z', 'ccdnum': 17, 'visit': 176846}] 2006 matches found
            Visits : [{'filter': 'z', 'ccdnum': 18, 'visit': 176846}] 1808 matches found
            Median value of the astrometric scatter - all magnitudes: 116.269301336 mas
            Astrometric scatter (median) - mag < 21.0 : 63.7 mas
            Median astrometric scatter 63.7 mas is larger than reference : 25.0 mas 
            

            I don't understand the purpose of defaultData in checkAstrometry.py. Is it necessary there? If so, please provide docstring. Also, its parameter 'repo' is not used. isExtended in checkAstrometry.py is also missing docstring.

            Please rewrite and clean up the commit history before merging. Many commits can be squashed, e.g.,

            • Rename files to camelCase convention.
            • Rename 'main' -> 'run'
            • Updated README with DECam validation.
            • Run validation on CCDs 10-18; expect >10000 matches.
            • Update Cfht->Decam in runDecamTest example
            • Fix whitespace and script to pass flake8+shellcode
            • Add obs_decam to list of pre-requisites

            Also see my last comment. I might have missed things, but you get the idea.

            Beyond the scope of this ticket, I see many measurement warnings like this:

            processCcdDecam.measurement WARNING: Error in base_Variance.measure on record 75950977596788670: The center is outside the Footprint of the source record
            

            Is this expected?

            Show
            hchiang2 Hsin-Fang Chiang added a comment - I was able to run runDecamTest.sh and obtained validating outputs as follows: 5019 sources in ccd: 10 5283 sources in ccd: 11 5192 sources in ccd: 12 5395 sources in ccd: 13 5171 sources in ccd: 14 5831 sources in ccd: 15 5726 sources in ccd: 16 5209 sources in ccd: 17 5428 sources in ccd: 18 48254 total sources in reference visit. Visits : [{ 'filter' : 'z' , 'ccdnum' : 10 , 'visit' : 176846 }] 1104 matches found Visits : [{ 'filter' : 'z' , 'ccdnum' : 11 , 'visit' : 176846 }] 1198 matches found Visits : [{ 'filter' : 'z' , 'ccdnum' : 12 , 'visit' : 176846 }] 1211 matches found Visits : [{ 'filter' : 'z' , 'ccdnum' : 13 , 'visit' : 176846 }] 503 matches found Visits : [{ 'filter' : 'z' , 'ccdnum' : 14 , 'visit' : 176846 }] 1008 matches found Visits : [{ 'filter' : 'z' , 'ccdnum' : 15 , 'visit' : 176846 }] 1710 matches found Visits : [{ 'filter' : 'z' , 'ccdnum' : 16 , 'visit' : 176846 }] 1961 matches found Visits : [{ 'filter' : 'z' , 'ccdnum' : 17 , 'visit' : 176846 }] 2006 matches found Visits : [{ 'filter' : 'z' , 'ccdnum' : 18 , 'visit' : 176846 }] 1808 matches found Median value of the astrometric scatter - all magnitudes: 116.269301336 mas Astrometric scatter (median) - mag < 21.0 : 63.7 mas Median astrometric scatter 63.7 mas is larger than reference : 25.0 mas I don't understand the purpose of defaultData in checkAstrometry.py . Is it necessary there? If so, please provide docstring. Also, its parameter 'repo' is not used. isExtended in checkAstrometry.py is also missing docstring. Please rewrite and clean up the commit history before merging. Many commits can be squashed, e.g., Rename files to camelCase convention. Rename 'main' -> 'run' Updated README with DECam validation. Run validation on CCDs 10-18; expect >10000 matches. Update Cfht->Decam in runDecamTest example Fix whitespace and script to pass flake8+shellcode Add obs_decam to list of pre-requisites Also see my last comment. I might have missed things, but you get the idea. Beyond the scope of this ticket, I see many measurement warnings like this: processCcdDecam.measurement WARNING: Error in base_Variance.measure on record 75950977596788670 : The center is outside the Footprint of the source record Is this expected?
            Hide
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

            Thanks very much for the comments! Added and incorporated.

            Show
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Thanks very much for the comments! Added and incorporated.
            Hide
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

            Merged to master.

            Show
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Merged to master.

              People

              Assignee:
              wmwood-vasey Michael Wood-Vasey [X] (Inactive)
              Reporter:
              wmwood-vasey Michael Wood-Vasey [X] (Inactive)
              Reviewers:
              Hsin-Fang Chiang
              Watchers:
              Hsin-Fang Chiang, Michael Wood-Vasey [X] (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Due:
                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.