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

Implement SphPoint

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw

      Description

      Implement RFC-131:

      Make afw::geom::Coord pure virtual (so it cannot be instantiated) and to add a new class afw::geom::SphPoint that represents a spherical point.

      Coord should contain a SphPoint rather than inherit from it. This allows Coord.offset and Coord.rotate to take a SphPoint argument, without the confusion of allowing the user to provide a Coord and wondering what is done with the coordinate system and epoch information.

      When implementing, look for opportunities to integrate with sphgeom package, such as borrowing SphPoint from there.

      Also be careful to integrate with the upcoming WCS overhaul.

      Only support distance if this makes the code simpler in some way (such as being provided by code borrowed from elsewhere). It is explicitly not a requirement.

        Attachments

          Issue Links

            Activity

            Hide
            jsick Jonathan Sick added a comment -

            I'm not able to critically dig into C++ doxygen for at least another few weeks, so I'll take myself off the review's critical path. I'll take a look at this module once I get back to Stack documentation work. In the meantime, I trust your judgement on this.

            Show
            jsick Jonathan Sick added a comment - I'm not able to critically dig into C++ doxygen for at least another few weeks, so I'll take myself off the review's critical path. I'll take a look at this module once I get back to Stack documentation work. In the meantime, I trust your judgement on this.
            Hide
            rowen Russell Owen added a comment - - edited

            Very nice work – clean, well tested and well documented code. I put comments on github. A few I'll repeat here:

            Please add an atPole method. I suspect the test should be latitude within epsilon of pi/2, rather than latitude not less than pi/2 based on two considerations (both of which may be wrong):

            • Getting reliable results from offset and rotated, even in extreme cases.
            • Being able to generate a point at the pole by providing a latitude specified in degrees or arcseconds.

            Allow longitude and latitude to be nan. Two important use cases are:

            • representing invalid values input to output from transforms
            • representing unknown boresight az/alt in VisitInfo

            I suggest that a point with a latitude of nan not be considered to be at the pole, so operations such as offset do not raise an exception. That will affect how atPole is implemented.

            I also suggest allowing +/-inf for longitude, not because I have a specific use case, but primarily for self-consistency: I think it would surprise users to be able to specify nan but not inf for longitude (clearly it is invalid for latitude). If you prefer to disallow it, that's fine.

            Please consider allowing nan in arguments to methods such as offset, to allow quiet propagation of nan when appropriate. The need is less obvious than for nan longitude and latitude, but it enhances self-consistency.

            Please consider adding an isFinite() method that returns true if and only if longitude and latitude are both finite (a request I did not make on github). I realize that classes such as Angle and Point do not have this, so feel free to reject this suggestion if you'd rather not add the method.

            In my opinion you should feel free to rename the class to SpherePoint or SphericalPoint or retain SphPoint, as you prefer. I think they are all sufficiently clear and unambiguous.

            Show
            rowen Russell Owen added a comment - - edited Very nice work – clean, well tested and well documented code. I put comments on github. A few I'll repeat here: Please add an atPole method. I suspect the test should be latitude within epsilon of pi/2 , rather than latitude not less than pi/2 based on two considerations (both of which may be wrong): Getting reliable results from offset and rotated , even in extreme cases. Being able to generate a point at the pole by providing a latitude specified in degrees or arcseconds. Allow longitude and latitude to be nan . Two important use cases are: representing invalid values input to output from transforms representing unknown boresight az/alt in VisitInfo I suggest that a point with a latitude of nan not be considered to be at the pole, so operations such as offset do not raise an exception. That will affect how atPole is implemented. I also suggest allowing +/-inf for longitude, not because I have a specific use case, but primarily for self-consistency: I think it would surprise users to be able to specify nan but not inf for longitude (clearly it is invalid for latitude). If you prefer to disallow it, that's fine. Please consider allowing nan in arguments to methods such as offset , to allow quiet propagation of nan when appropriate. The need is less obvious than for nan longitude and latitude, but it enhances self-consistency. Please consider adding an isFinite() method that returns true if and only if longitude and latitude are both finite (a request I did not make on github). I realize that classes such as Angle and Point do not have this, so feel free to reject this suggestion if you'd rather not add the method. In my opinion you should feel free to rename the class to SpherePoint or SphericalPoint or retain SphPoint , as you prefer. I think they are all sufficiently clear and unambiguous.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Russell Owen, I've made most of the requested style changes, and also added support for nan/inf values. It's also now the more pronounceable SpherePoint.

            Show
            krzys Krzysztof Findeisen added a comment - Russell Owen , I've made most of the requested style changes, and also added support for nan/inf values. It's also now the more pronounceable SpherePoint .
            Hide
            rowen Russell Owen added a comment - - edited

            Overall this is really nice code. Clear, well tested and well documented. Thank you for the changes, including allowing NaN long and lat.

            I did request a few more tests (despite the already impressively large suite of tests) and a few minor changes to the documentation (despite the already excellent documentation). Once you have had a chance to consider my requests and deal with them as you see fit, please rebase the commits, run Jenkins (both py2 and py3) and merge.

            Show
            rowen Russell Owen added a comment - - edited Overall this is really nice code. Clear, well tested and well documented. Thank you for the changes, including allowing NaN long and lat. I did request a few more tests (despite the already impressively large suite of tests) and a few minor changes to the documentation (despite the already excellent documentation). Once you have had a chance to consider my requests and deal with them as you see fit, please rebase the commits, run Jenkins (both py2 and py3) and merge.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Fixed and merged.

            Show
            krzys Krzysztof Findeisen added a comment - Fixed and merged.

              People

              Assignee:
              krzys Krzysztof Findeisen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Russell Owen
              Watchers:
              Jim Bosch, John Parejko, Jonathan Sick, Krzysztof Findeisen, Paul Price, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.