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

Remove all use of the geom package

    XMLWordPrintable

    Details

    • Story Points:
      3
    • Epic Link:
    • Sprint:
      AP S18-4, AP S18-6
    • Team:
      Alert Production

      Description

      Our stack has a very few bits of code that still use the geom package. In particular they use the convexHull function. geom has been superseded by sphgeom (which is written in C++ instead of python) and contains ConvexPolygon.convexHull. Update the code, paying careful attention to API differences, if any.

      This is driven by a desire to repurpose the geom package for afw geometry primitives (RFC-460) and to retire obsolete and unmaintained code.

        Attachments

          Issue Links

          is blocked by

          Improvement - An improvement or enhancement to an existing feature or task. DM-14255 Add ConvexPolygon.intersects and related methods

          • Done

          Improvement - An improvement or enhancement to an existing feature or task. DM-13905 Change SpherePoint.getVector to return a sphgeom UnitVector3d

          • Done
          is triggered by

          RFC - An issue whose primary purpose is to advertise an intent and allow stakeholders to comment. RFC-460 Replace the lsst.geom package with components from lsst.afw.geom

          • Implemented
          is triggering

          Bug - A problem which impairs or prevents the functions of the product. DM-14528 w_2018_19 & w_2018_20 coaddDriver is broken

          • Done
          relates to

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-13727 Remove skypix from stack dependencies

          • Done

            Activity

            Ascending order - Click to sort in descending order
            5 older comments
            Hide
            Permalink
            rowen Russell Owen added a comment - - edited

            Paul Price I took your excellent advice to centralize the code. This consisted of two changes:

            1) Made imageOverlapsTract a public function in meas_base. This should be rendered obsolete by the gen3 butler, but until then it's useful to have just one copy, as you said.

            2) Added the ability to get a sky polygon from lsst.skymap.TractInfo and PatchInfo (for both inner and outer regions of each). Jim Bosch recently added something like this to skymap and I negotiated a change to add flexibility and clarity. I also added a unit test for that.

            I also further refined imageOverlapsTract a bit, e.g. converting all image corners from pixels to sky in a single call to SkyWcs.pixelToSky.

            Please have another look. skymap changed significantly, meas_base and jointcal both changed a bit.

            Show
            rowen Russell Owen added a comment - - edited Paul Price I took your excellent advice to centralize the code. This consisted of two changes: 1) Made imageOverlapsTract a public function in meas_base . This should be rendered obsolete by the gen3 butler, but until then it's useful to have just one copy, as you said. 2) Added the ability to get a sky polygon from lsst.skymap.TractInfo and PatchInfo (for both inner and outer regions of each). Jim Bosch recently added something like this to skymap and I negotiated a change to add flexibility and clarity. I also added a unit test for that. I also further refined imageOverlapsTract a bit, e.g. converting all image corners from pixels to sky in a single call to SkyWcs.pixelToSky . Please have another look. skymap changed significantly, meas_base and jointcal both changed a bit.
            Hide
            Permalink
            price Paul Price added a comment -

            Could you please factor out the code duplication in the skymap tests you added?

            Show
            price Paul Price added a comment - Could you please factor out the code duplication in the skymap tests you added?
            Hide
            Permalink
            rowen Russell Owen added a comment - - edited

            Paul Price good suggestion. I centralized the tests in BaseSkyMapTestCase and factored common code into other methods of that class.

            I also found test_equatMap.py was not using BaseSkyMapTestCase and changed that (requiring some work in the base class test case). This exposed a bug in BaseSkyMap that I fixed. Unfortunately I don't know how to unit test it because it relies on float equality. Fortunately test_equatMap.py seems to trigger it reliably.

            Please have yet another look. The only changes are in skymap. Sorry for bouncing it back to you. It seems a poor way of thanking you for your good suggestions. But the changes are large enough that I think another quick look, at least, is wise.

            I have not rebased the changes yet, in the interest of making it easier to see the new changes.

            Show
            rowen Russell Owen added a comment - - edited Paul Price good suggestion. I centralized the tests in BaseSkyMapTestCase and factored common code into other methods of that class. I also found test_equatMap.py was not using BaseSkyMapTestCase and changed that (requiring some work in the base class test case). This exposed a bug in BaseSkyMap that I fixed. Unfortunately I don't know how to unit test it because it relies on float equality. Fortunately test_equatMap.py seems to trigger it reliably. Please have yet another look. The only changes are in skymap . Sorry for bouncing it back to you. It seems a poor way of thanking you for your good suggestions. But the changes are large enough that I think another quick look, at least, is wise. I have not rebased the changes yet, in the interest of making it easier to see the new changes.
            Hide
            Permalink
            price Paul Price added a comment -

            Very nice!

            Show
            price Paul Price added a comment - Very nice!
            Hide
            Permalink
            rowen Russell Owen added a comment - - edited

            Paul Price Thank you for the very helpful review. I'm sorry the review turned into a marathon, but the code is far better for your suggested changes.

            Show
            rowen Russell Owen added a comment - - edited Paul Price Thank you for the very helpful review. I'm sorry the review turned into a marathon, but the code is far better for your suggested changes.

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Paul Price
              Watchers:
              Jim Bosch, Paul Price, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.