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

Add assertXNearlyEqual to afw

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      5
    • Sprint:
      Science Pipelines DM-S15-1, Science Pipelines DM-S15-4
    • Team:
      Alert Production

      Description

      We often want to compare two WCS for approximate equality. afw/image/testUtils has similar functions to compare images and masks and I would like to add one for WCS

      This ended up being expanded to adding functions for many afw classes (not yet including image-like classes, though existing functions in image/testUtils for that purpose should probably be wrapped or rewritten on a different ticket)

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            If you have time, please have a look. It's a small addition to afw on tickets/DM-2193. I decided to make it look more like a normal unittest assert than the other items in the file.

            One of the tests in meas_astrom will be converted to use this when it is available. I'm sure other tests could also take advantage of it.

            Show
            rowen Russell Owen added a comment - If you have time, please have a look. It's a small addition to afw on tickets/ DM-2193 . I decided to make it look more like a normal unittest assert than the other items in the file. One of the tests in meas_astrom will be converted to use this when it is available. I'm sure other tests could also take advantage of it.
            Hide
            price Paul Price added a comment -

            Review was completed on Github (https://github.com/lsst/afw/pull/12). Concerns include:

            • Inclusion of changes outside the declared purpose of the commit.
            • Removal of useful comments.
            • Naming of the function.
            • Various small layout issues (spaces around operators, alignment).
            Show
            price Paul Price added a comment - Review was completed on Github ( https://github.com/lsst/afw/pull/12 ). Concerns include: Inclusion of changes outside the declared purpose of the commit. Removal of useful comments. Naming of the function. Various small layout issues (spaces around operators, alignment).
            Hide
            rowen Russell Owen added a comment -

            Thank you for your review. Unfortunately I think I'd best hold off merging this until RFC-28 is resolved.

            Show
            rowen Russell Owen added a comment - Thank you for your review. Unfortunately I think I'd best hold off merging this until RFC-28 is resolved.
            Hide
            krughoff Simon Krughoff added a comment -

            This needs a half day more work to bring it in line with RFC-28.

            Show
            krughoff Simon Krughoff added a comment - This needs a half day more work to bring it in line with RFC-28 .
            Hide
            rowen Russell Owen added a comment -

            I brought assertWcssNearlyEqualOverBBox in line with RFC-28, though only the assertion, not the predicate because it is too different. The assertion function tests all points and reports the largest difference (on sky and in pixels) if there is an error. A predicate should fail as soon as an error is detected.

            I also implemented assertions for Angle, Box2, pairs (e.g. Point and Extent) and Coord. Again, I did not implement predicates because I am not convinced they are useful and it is clumsy to try to make them work as predicates and assertions (as K-T suggested in RFC-28). I am pretty sure that a function that reports the measured difference would be more useful, but in many cases that is already implemented in some other way (e.g. Coord.angularSeparation). I felt it best to wait for a good use case before adding anything beyond the assertions.

            The assertion functions and a unit test for them are in afw on tickets/DM-2193.

            In addition, I modified two unit tests in meas_astrom on tickets/DM-2193 to use assertWcssNearlyEqualOverBBox

            Show
            rowen Russell Owen added a comment - I brought assertWcssNearlyEqualOverBBox in line with RFC-28 , though only the assertion, not the predicate because it is too different. The assertion function tests all points and reports the largest difference (on sky and in pixels) if there is an error. A predicate should fail as soon as an error is detected. I also implemented assertions for Angle, Box2, pairs (e.g. Point and Extent) and Coord. Again, I did not implement predicates because I am not convinced they are useful and it is clumsy to try to make them work as predicates and assertions (as K-T suggested in RFC-28 ). I am pretty sure that a function that reports the measured difference would be more useful, but in many cases that is already implemented in some other way (e.g. Coord.angularSeparation). I felt it best to wait for a good use case before adding anything beyond the assertions. The assertion functions and a unit test for them are in afw on tickets/ DM-2193 . In addition, I modified two unit tests in meas_astrom on tickets/ DM-2193 to use assertWcssNearlyEqualOverBBox
            Hide
            rowen Russell Owen added a comment -

            Could you please have a look at the concept, at least? This doesn't quite match what you asked for in RFC-28 so I want to be sure it will suffice. If you don't have the time or inclination to review the code in detail then let I will be happy to hand it to somebody else.

            Show
            rowen Russell Owen added a comment - Could you please have a look at the concept, at least? This doesn't quite match what you asked for in RFC-28 so I want to be sure it will suffice. If you don't have the time or inclination to review the code in detail then let I will be happy to hand it to somebody else.
            Hide
            ktl Kian-Tat Lim added a comment -

            Minor comments on PR; could provide an early-exit comparison along with the assertion function, at the cost of somewhat messier code.

            Show
            ktl Kian-Tat Lim added a comment - Minor comments on PR; could provide an early-exit comparison along with the assertion function, at the cost of somewhat messier code.

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Kian-Tat Lim
              Watchers:
              Kian-Tat Lim, Paul Price, Russell Owen, Simon Krughoff
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: