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

Coord should be abstract

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None
    • Team:
      Data Release Production

      Description

      afw.coord.Coord appears to be meant as an abstract base class, but it can be instantiated. When a Coord is instantiated it's not clear what the various coordinate conversion methods should do. I think it would be best to make sure that Coord could not be instantiated.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Russell Owen, I was looking at this in connection with DM-5529, and there's a problem: Coord::transform accepts two arbitrary Coords, and returns a newly constructed Coord. From the documentation for this method, it's not possible to make it return one of EclipticCoord, Fk5Coord, GalacticCoord, IcrsCoord, or TopocentricCoord – there needs to be a "generic coordinate system" class to support arbitrary arguments to transform.

            I propose that abstracting Coord be moved from DM-5529 to here, as it's a nontrivial change, and that creation of a new subclass (GenericCoord?) with capabilities similar to the current Coord be included as part of this ticket.

            EDIT: actually, now that I think of it, it's not really clear how transform should be defined for subclasses of Coord... what happens if poleTo is of a different type, for instance?

            Show
            krzys Krzysztof Findeisen added a comment - - edited Russell Owen , I was looking at this in connection with DM-5529 , and there's a problem: Coord::transform accepts two arbitrary Coords , and returns a newly constructed Coord . From the documentation for this method, it's not possible to make it return one of EclipticCoord , Fk5Coord , GalacticCoord , IcrsCoord , or TopocentricCoord – there needs to be a "generic coordinate system" class to support arbitrary arguments to transform . I propose that abstracting Coord be moved from DM-5529 to here, as it's a nontrivial change, and that creation of a new subclass ( GenericCoord ?) with capabilities similar to the current Coord be included as part of this ticket. EDIT: actually, now that I think of it, it's not really clear how transform should be defined for subclasses of Coord ... what happens if poleTo is of a different type, for instance?
            Hide
            rowen Russell Owen added a comment - - edited

            I agree that making Coord an abstract base class should be moved to a new ticket and implemented only after SphPoint is available.

            I suspect we may never make Coord an abstract base class. It is more likely that Coord will end up being a single class that contains a SphPoint and some metadata (e.g. the coordinate system and epoch), and that it will lose its ability to transform between coordinate systems or use AST internally to provide that ability.

            Meanwhile, best to implement SphPoint and leave Coord alone.

            Show
            rowen Russell Owen added a comment - - edited I agree that making Coord an abstract base class should be moved to a new ticket and implemented only after SphPoint is available. I suspect we may never make Coord an abstract base class. It is more likely that Coord will end up being a single class that contains a SphPoint and some metadata (e.g. the coordinate system and epoch), and that it will lose its ability to transform between coordinate systems or use AST internally to provide that ability. Meanwhile, best to implement SphPoint and leave Coord alone.
            Hide
            tjenness Tim Jenness added a comment - - edited

            To second Russell Owen, during the Astropy discussions we came to the conclusion that the C++ code never does coordinate conversion so there is no reason to support Galactic, Ecliptic, FK5 and the rest. If you want conversion you can do it in Python with Astropy. When I ran the astropy coordinate conversion comparison routines (which checks many different libraries) the LSST code was not that great (especially for Galactic).

            Show
            tjenness Tim Jenness added a comment - - edited To second Russell Owen , during the Astropy discussions we came to the conclusion that the C++ code never does coordinate conversion so there is no reason to support Galactic, Ecliptic, FK5 and the rest. If you want conversion you can do it in Python with Astropy. When I ran the astropy coordinate conversion comparison routines (which checks many different libraries) the LSST code was not that great (especially for Galactic).
            Hide
            rowen Russell Owen added a comment -

            Implemented as part of DM-11162 eliminate the Coord classes

            Show
            rowen Russell Owen added a comment - Implemented as part of DM-11162 eliminate the Coord classes

              People

              • Assignee:
                Unassigned
                Reporter:
                rowen Russell Owen
                Watchers:
                Krzysztof Findeisen, Russell Owen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel