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

Stop using astrometry_net by default

    Details

      Description

      There is code in pipe_tasks that uses astrometry_net to load catalogs and fit astrometric solutions. This ticket is to move to the new reference catalog format and newer astrometry fitter.

      Note that when this is done in pipe_tasks that we can remove meas_extensions_astrometryNet as a dependency.

      I suggest this wait on DM-2186, though that is not strictly necessary.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            Jenkins passed (after some mucking with the demo)!

            Paul Price: are you able to review this ticket (-800 or so lines, +20)? It's 5 obs packages, the demo, and pipe_tasks. Jenkins passed including ci_hsc, but I'm not sure where else to check.

            pipe_tasks PR, since Jira isn't picking it up: https://github.com/lsst/pipe_tasks/pull/278

            Show
            Parejkoj John Parejko added a comment - Jenkins passed (after some mucking with the demo)! Paul Price : are you able to review this ticket (-800 or so lines, +20)? It's 5 obs packages, the demo, and pipe_tasks . Jenkins passed including ci_hsc, but I'm not sure where else to check. pipe_tasks PR, since Jira isn't picking it up: https://github.com/lsst/pipe_tasks/pull/278
            Hide
            price Paul Price added a comment -

            A test is being removed from pipe_tasks that isn't being reinstated somewhere. I am concerned that this is violating Robert Lupton's requirement "that we keep astrometry.net alive in reality": after DM-18036, there won't be anything that actually exercises the astrometry.net matcher under ProcessCcdTask.

            I find the commit messages to be lacking. Most contain the text "Remove no longer necessary retarget", which doesn't read well, and does not allow someone perusing the history to easily understand what's going on. I would prefer to see commit messages that say something like:

            config: remove retarget to LoadIndexedReferenceObjectsTask
             
            It is now the default setting.
            

            That has multiple advantages over what's present now:
            1. It specifies the area of the codebase that's being changed (config).
            2. It specifies exactly what is being changed (LoadIndexedReferenceObjectsTask).
            3. It specifies why it's being changed (it's now the default).

            You may think this is overkill for such a simple ticket, but it's important to maintain the habit of providing that information for all tickets.

            Show
            price Paul Price added a comment - A test is being removed from pipe_tasks that isn't being reinstated somewhere. I am concerned that this is violating Robert Lupton 's requirement "that we keep astrometry.net alive in reality": after DM-18036 , there won't be anything that actually exercises the astrometry.net matcher under ProcessCcdTask . I find the commit messages to be lacking. Most contain the text "Remove no longer necessary retarget", which doesn't read well, and does not allow someone perusing the history to easily understand what's going on. I would prefer to see commit messages that say something like: config: remove retarget to LoadIndexedReferenceObjectsTask   It is now the default setting. That has multiple advantages over what's present now: 1. It specifies the area of the codebase that's being changed ( config ). 2. It specifies exactly what is being changed ( LoadIndexedReferenceObjectsTask ). 3. It specifies why it's being changed (it's now the default). You may think this is overkill for such a simple ticket, but it's important to maintain the habit of providing that information for all tickets.
            Hide
            Parejkoj John Parejko added a comment -

            Note that RFC-562 was adopted: a.net will not be supported by gen3 middleware.

            Show
            Parejkoj John Parejko added a comment - Note that RFC-562 was adopted: a.net will not be supported by gen3 middleware.
            Hide
            Parejkoj John Parejko added a comment - - edited

            Fair point on the commit messages: I've updated the obs packages to something similar to the one you give above.

            Getting to the point where no code run by CI exercises the a.net matcher is a requirement for implementing RFC-562. I think the stack demo is the last bit of CI code that uses a.net refcats.

            Show
            Parejkoj John Parejko added a comment - - edited Fair point on the commit messages: I've updated the obs packages to something similar to the one you give above. Getting to the point where no code run by CI exercises the a.net matcher is a requirement for implementing RFC-562 . I think the stack demo is the last bit of CI code that uses a.net refcats.
            Hide
            Parejkoj John Parejko added a comment -

            Thank you for the quick review: the obs package commit messages should be better now.

            Merged and done.

            Show
            Parejkoj John Parejko added a comment - Thank you for the quick review: the obs package commit messages should be better now. Merged and done.

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                rowen Russell Owen
                Reviewers:
                Paul Price
                Watchers:
                Hsin-Fang Chiang, John Parejko, Paul Price, Robert Lupton, Russell Owen, Simon Krughoff, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel