Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-367

Add (double, double, units) constructor to SpherePoint and an associated getter

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      It would be very convenient to have a SpherePoint constructor that takes a pair of doubles and a units argument. Coord has something similar and it is widely used, e.g. in unit tests. Specifically, I propose:

      SpherePoint(double long, double lat, AngleUnit units)
      

      This is intentionally different than Coord, which offers Coord(Point2D raDec, AngleUnit units=degrees). I prefer my proposed interface for several reasons:

      • Point2D is intrinsically Cartesian, so I think it is a mistake to be able to construct a SpherePoint from one.
      • In most cases where the Coord constructor is used, the values are specified directly, leading to this sort of clumsy code: coord = afwCoord.Coord(afwGeom.Point2D(45,0, 10.23), afwGeom.degrees). It is simpler to say point = afwGeom.SpherePoint(45.0, 10.23, afwGeom.degrees).
      • Defaulting the units argument makes it difficult to tell what the code does, especially if RA and Dec are small. Furthermore, given our bias towards radians, I would expect that to be the default, if we had one.

      Another proposed difference is that the units argument of the SpherePoint constructor always applies to both longitude and latitude. Coord usually does this, but treats the hours unit specially: RA is hours and Dec is degrees. I can see why that was done, but I think it is too clever and surprising to want to emulate it for SpherePoint, especially since the latter is not intrinsically RA, Dec anyway. I prefer simple consistency. If one really wants to provide hours and degrees one can provide the appropriate angles as arguments.

      In addition, I propose a corresponding method to get the longitude and latitude of a SpherePoint as a pair of floats in the specified units: SpherePoint.getNumbers(units) -> std::pair<double, double>. Other name suggestions are welcome.

      This is analogous to Coord.getPosition(units=degrees) with the same justification for the differences. I am less positive we need this (and am not thrilled with the name), but Coord.getPosition is fairly widely used, and it sometimes results in clearer code than retrieving each angle and extract the numeric value in the specified units.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment -

            Yes, I'll concede that that pattern of overloads is a bad idea.

            Show
            krzys Krzysztof Findeisen added a comment - Yes, I'll concede that that pattern of overloads is a bad idea.
            Hide
            Parejkoj John Parejko added a comment -

            I like getSphericalCoordinates(unit). Less confusing with getLongitude/getLatitude, and since SpherePoint is designed for points on the sky, it's pretty clear what you would get.

            Show
            Parejkoj John Parejko added a comment - I like getSphericalCoordinates(unit) . Less confusing with getLongitude/getLatitude , and since SpherePoint is designed for points on the sky, it's pretty clear what you would get.
            Hide
            tjenness Tim Jenness added a comment -

            Where are we on this RFC?

            Show
            tjenness Tim Jenness added a comment - Where are we on this RFC?
            Hide
            tjenness Tim Jenness added a comment -

            Ping.

            Show
            tjenness Tim Jenness added a comment - Ping.
            Hide
            rowen Russell Owen added a comment -

            Adopted as originally expressed but without a way to retrieve longitude and latitude as a pair of floats. The constructor is more important and there is no ambiguity about what it should look like. I am not sufficiently happy with any of the suggested names for methods to return a pair of floats to want to pick one at this time. If we find we really need it we can try another RFC.

            Show
            rowen Russell Owen added a comment - Adopted as originally expressed but without a way to retrieve longitude and latitude as a pair of floats. The constructor is more important and there is no ambiguity about what it should look like. I am not sufficiently happy with any of the suggested names for methods to return a pair of floats to want to pick one at this time. If we find we really need it we can try another RFC.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel