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

Fix jointcal handling of coordinate errors

    Details

      Description

      Since we didn't have a refcat with coordinate errors, I couldn't write a test of jointcal's use of them. And thus I got it wrong: disabling config.astrometryReferenceErr resulted in an exception because the error field is coord_raErr not coord_ra_err!

      Unrelatedly, but I might as well fix it as part of this work: it looks like jointcal is trying to apply colorterms when loading the astrometry reference catalog. That is both not necessary, and unhelpful when the astrometry refcat doesn't have useful photometry (e.g. Gaia). Might as well fix that on this ticket, too.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            Lauren MacArthur: do you mind doing this small review? Sadly, I still don't have a test refcat I can use to demonstrate the fix, but it did work for the sample Gaia refcat that I generated that has real coordinate errors.

            Show
            Parejkoj John Parejko added a comment - Lauren MacArthur : do you mind doing this small review? Sadly, I still don't have a test refcat I can use to demonstrate the fix, but it did work for the sample Gaia refcat that I generated that has real coordinate errors.
            Show
            Parejkoj John Parejko added a comment - Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29870/pipeline
            Hide
            lauren Lauren MacArthur added a comment - - edited

            I'm not sure that your "Sadly..." phrase is self-consistent (you don't have a test refcat, but you used a refcat to test it on?!) It would be nice if you could provide a few more details on how you came to the conclusion that things are indeed "working" properly with your Gaia refcat test.

            Regardless, some minor comments on the PR (may have come as two separate comments rather than a single review as I'd intended...JIRA + github seem to be acting strangely for me today!) Otherwise, good to go.

            Show
            lauren Lauren MacArthur added a comment - - edited I'm not sure that your "Sadly..." phrase is self-consistent (you don't have a test refcat, but you used a refcat to test it on?!) It would be nice if you could provide a few more details on how you came to the conclusion that things are indeed "working" properly with your Gaia refcat test. Regardless, some minor comments on the PR (may have come as two separate comments rather than a single review as I'd intended...JIRA + github seem to be acting strangely for me today!) Otherwise, good to go.
            Hide
            Parejkoj John Parejko added a comment -

            I tried running jointcal using the first Gaia DR2 refcat I had generated, to test the refcat itself (which turns out to be broken) and to test jointcal's use of the refcat coord error fields (which turned up this bug). It's a one-off, though, since I still don't have a working Gaia DR2 refcat available (DM-19473). Once I do, I plan to update testdata_jointcal to use it, and then we'll have proper tests of this block.

            For now, I'll have to live with knowing I demonstrated that it worked in one instance.

            Show
            Parejkoj John Parejko added a comment - I tried running jointcal using the first Gaia DR2 refcat I had generated, to test the refcat itself (which turns out to be broken) and to test jointcal's use of the refcat coord error fields (which turned up this bug). It's a one-off, though, since I still don't have a working Gaia DR2 refcat available ( DM-19473 ). Once I do, I plan to update testdata_jointcal to use it, and then we'll have proper tests of this block. For now, I'll have to live with knowing I demonstrated that it worked in one instance.
            Hide
            Parejkoj John Parejko added a comment -

            Thanks for the quick review.

            Merged and done.

            Show
            Parejkoj John Parejko added a comment - Thanks for the quick review. Merged and done.

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                Lauren MacArthur
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Lauren MacArthur
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel