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

Make SkyWcs transform to IcrsCoord instead of SpherePoint

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      5
    • Epic Link:
    • Sprint:
      Alert Production S17 - 6
    • Team:
      Alert Production

      Description

      At present SkyWcs is a subclass of Transform<Point2Endpoint, SpherePointEndpoint> and this means that pixelToSky transforms to SpherePoint. However SkyWcs is always normalized to ICRS and so pixelToSky should probably produce IcrsCoord.

      The simplest way to change this is to change SpherePointEndpoint to IcrsCoordEndpoint. One obvious alternative is to keep both and change the endpoint class SkyWcs uses. However, we have no identified need for Transform to transform to SpherePoint (nor IcrsCoord except as a base class for SkyWcs). Furthermore if we have N Endpoint classes then we instantiate N^2 Transform classes. So I propose to simply rename for now.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            Unfortunately this change introduced a circular dependency because Transform and SkyWcs are defined in afw.geom but the now use IcrsCoord and afw.coord depends on afw.geom for Angle. I think this reflects a basic issue with our sub-packages: low-level things like Angle perhaps do not belong with higher-level things like Transform.

            It also somewhat raises the issue of do we really need SpherePoint, IcrsCoord and the other Coord classes. I'm skeptical we need any Coord classes other than IcrsCoord since table only handles the latter and most of our code assumes the same of Wcs. In fact the new SkyWcs class that will replace Wcs is always normalized to Icrs. We can use AstroPy for coordinate conversions, e.g. when ingesting external catalogs.

            That leaves SpherePoint and IcrsCoord which are very similar in practice, though they mean slightly different things. SpherePoint is used when we want a longitude/latitude angle pair without an associated coordinate system. So far we only have identified a few uses for this. For example Coord.transform takes two of these as arguments to describe two poles (though was written before SpherePoint so its arguments are Coord}}s). We probably do want both, but {{SpherePoint may not get much use unless we refactor IcrsCoord to use it (inherit from or contain it).

            We don't presently have an identified need for Transforms that convert to/from SpherePoint (or IcrsCoord either, other than as a base class for SkyWcs), so for now I replaced SpherePointEndpoint with IcrsCoordEndpoint.

            Show
            rowen Russell Owen added a comment - Unfortunately this change introduced a circular dependency because Transform and SkyWcs are defined in afw.geom but the now use IcrsCoord and afw.coord depends on afw.geom for Angle . I think this reflects a basic issue with our sub-packages: low-level things like Angle perhaps do not belong with higher-level things like Transform . It also somewhat raises the issue of do we really need SpherePoint , IcrsCoord and the other Coord classes. I'm skeptical we need any Coord classes other than IcrsCoord since table only handles the latter and most of our code assumes the same of Wcs . In fact the new SkyWcs class that will replace Wcs is always normalized to Icrs . We can use AstroPy for coordinate conversions, e.g. when ingesting external catalogs. That leaves SpherePoint and IcrsCoord which are very similar in practice, though they mean slightly different things. SpherePoint is used when we want a longitude/latitude angle pair without an associated coordinate system. So far we only have identified a few uses for this. For example Coord.transform takes two of these as arguments to describe two poles (though was written before SpherePoint so its arguments are Coord}}s). We probably do want both, but {{SpherePoint may not get much use unless we refactor IcrsCoord to use it (inherit from or contain it). We don't presently have an identified need for Transforms that convert to/from SpherePoint (or IcrsCoord either, other than as a base class for SkyWcs ), so for now I replaced SpherePointEndpoint with IcrsCoordEndpoint .
            Hide
            rowen Russell Owen added a comment -

            The changes are straightforward but feel free to comment on the issues I raised. I don't want to shove this through until we've had time to think about it.

            Show
            rowen Russell Owen added a comment - The changes are straightforward but feel free to comment on the issues I raised. I don't want to shove this through until we've had time to think about it.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment - - edited

            Some minor comments on the PR.

            As for the circular dependencies. If the only dependency is through afw::geom::Angle how about refactoring that out of afw entirely and use sphgeom::Angle instead? I think Jim Bosch mentioned that is something we want to do anyway with the primitives. But I might be mistaken. Otherwise we should perhaps factor out the primitives into a separate afw::primitives namespace?

            With respect to the other Coord subclasses. It depends if we need them from C++ I suppose. If not, we could drop them and do all conversions with astropy I guess. But I find it hard to believe this is truly the case.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - - edited Some minor comments on the PR. As for the circular dependencies. If the only dependency is through afw::geom::Angle how about refactoring that out of afw entirely and use sphgeom::Angle instead? I think Jim Bosch mentioned that is something we want to do anyway with the primitives. But I might be mistaken. Otherwise we should perhaps factor out the primitives into a separate afw::primitives namespace? With respect to the other Coord subclasses. It depends if we need them from C++ I suppose. If not, we could drop them and do all conversions with astropy I guess. But I find it hard to believe this is truly the case.
            Hide
            rowen Russell Owen added a comment - - edited

            Regarding Coord subclasses: we're readying an RFC but I'm not sure this work needs to be held up by it.

            Moving Angle deeper sounds like a great idea (I'd like to see that done for Point and Extent as well). The question is whether we should hold up this merge for that or live with the circular dependency now. The circular dependency manifests as the inability to use IcrsCoordEndpoint or SkyWcs from Python without importing lsst.afw.coord; since those are used with IcrsCoord I think it is tolerable. What do you think?

            Show
            rowen Russell Owen added a comment - - edited Regarding Coord subclasses: we're readying an RFC but I'm not sure this work needs to be held up by it. Moving Angle deeper sounds like a great idea (I'd like to see that done for Point and Extent as well). The question is whether we should hold up this merge for that or live with the circular dependency now. The circular dependency manifests as the inability to use IcrsCoordEndpoint or SkyWcs from Python without importing lsst.afw.coord ; since those are used with IcrsCoord I think it is tolerable. What do you think?
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            I think we should merge it and file a separate ticket for cleaning up the circular dependency (perhaps file that ticket first and then also add a comment pointing to it, above the commented out import) before merging). Then perhaps RFC moving those primitives somewhere else. Although I don't know if sphgeom is the best place for those (except for Angle which clearly belongs there).

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - I think we should merge it and file a separate ticket for cleaning up the circular dependency (perhaps file that ticket first and then also add a comment pointing to it, above the commented out import) before merging). Then perhaps RFC moving those primitives somewhere else. Although I don't know if sphgeom is the best place for those (except for Angle which clearly belongs there).
            Hide
            rowen Russell Owen added a comment -

            Pim Schellart [X] good suggestion. I have posted DM-10999 (along with two related RFCs) and will reference it in the pybind11 wrapper.

            Show
            rowen Russell Owen added a comment - Pim Schellart [X] good suggestion. I have posted DM-10999 (along with two related RFCs) and will reference it in the pybind11 wrapper.
            Hide
            rowen Russell Owen added a comment -

            I rebased the commits to a first commit that applies small clang-format changes and the second commit that changes the code.

            Show
            rowen Russell Owen added a comment - I rebased the commits to a first commit that applies small clang-format changes and the second commit that changes the code.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Pim Schellart [X] (Inactive)
                Watchers:
                John Parejko, Krzysztof Findeisen, Pim Schellart [X] (Inactive), Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel