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

assertWcsNearlyEqualOverBBox and friends is too hard to use as a free function

    XMLWordPrintable

    Details

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

      Description

      assertWcsNearlyEqualOverBBox and similar functions elsewhere in afw were written to be methods of lsst.utils.tests.TestCase, so their first argument is a testCase. This is fine for use in unit tests, but a hassle to use as free functions because the user must provide a testCase argument (though it need only be a trivial class with a fail(self, msgStr) method). Worse, that minimal requirement is not documented, so technically providing a simple mock test case is unsafe.

      I have two proposals:

      • Document the fact that testCase need only support fail(self, msgStr). This makes it clear how to safely use these functions as free functions.
      • Allow testCase to be None, in which case RuntimeError is raised. That makes these functions even easier to use as free functions.

        Attachments

          Issue Links

            Activity

            Hide
            ktl Kian-Tat Lim added a comment -

            If the free function is really useful, I think I'd rather have a separate free function (that might return a boolean rather than raise) and a test-specific assert* function that uses it, rather than overloading too much into the parameters of the latter. We should keep all the assert* functions as similar as possible.

            Show
            ktl Kian-Tat Lim added a comment - If the free function is really useful, I think I'd rather have a separate free function (that might return a boolean rather than raise) and a test-specific assert* function that uses it, rather than overloading too much into the parameters of the latter. We should keep all the assert* functions as similar as possible.
            Hide
            tjenness Tim Jenness added a comment -

            Some of this was discussed in RFC-28. Two functions seems reasonable now that we are clearly using the function more widely.

            Show
            tjenness Tim Jenness added a comment - Some of this was discussed in RFC-28 . Two functions seems reasonable now that we are clearly using the function more widely.
            Hide
            rowen Russell Owen added a comment - - edited

            Based on the comments I have added a free predicate function wcsNearlyEqualOverBBox that uses short circuit evalutation. That and the existing related assert are now implemented in terms of a private function _compareWcsOverBBox.

            I also updated the documentation for testCase to explicitly list its requirements, making it safe to mock if desired. In one case (the bbox test) a trivial modification was required to make it true that only testCase.fail was called.

            I did not attempt to add predicates for any other asserts. I'd rather leave that for another ticket.

            This work is in afw on tickets/DM-3347.

            I did not attempt to allow testCase=None, as that was rightly pointed out as less than ideal. For the record, it would probably be easy to change our decorator such that testCase=None was turned into an instance of TestCase. That would provide consistency, but I think it would make the code for the asserts too difficult to understand ("how does None support fail(msgStr)?"). Best to just drop the whole idea.

            Show
            rowen Russell Owen added a comment - - edited Based on the comments I have added a free predicate function wcsNearlyEqualOverBBox that uses short circuit evalutation. That and the existing related assert are now implemented in terms of a private function _compareWcsOverBBox. I also updated the documentation for testCase to explicitly list its requirements, making it safe to mock if desired. In one case (the bbox test) a trivial modification was required to make it true that only testCase.fail was called. I did not attempt to add predicates for any other asserts. I'd rather leave that for another ticket. This work is in afw on tickets/ DM-3347 . I did not attempt to allow testCase=None, as that was rightly pointed out as less than ideal. For the record, it would probably be easy to change our decorator such that testCase=None was turned into an instance of TestCase. That would provide consistency, but I think it would make the code for the asserts too difficult to understand ("how does None support fail(msgStr)?"). Best to just drop the whole idea.
            Hide
            ktl Kian-Tat Lim added a comment -

            Sorry I'm a bit late with this, but neither GitHub nor JIRA seems to be able to see that branch. Did you push?

            Show
            ktl Kian-Tat Lim added a comment - Sorry I'm a bit late with this, but neither GitHub nor JIRA seems to be able to see that branch. Did you push?
            Hide
            rowen Russell Owen added a comment -

            K-T you are right: I had not pushed. My apologies. I just did so now.

            Show
            rowen Russell Owen added a comment - K-T you are right: I had not pushed. My apologies. I just did so now.
            Hide
            ktl Kian-Tat Lim added a comment -

            Two small comments on the PR, one on the code and one on the documentation.

            Show
            ktl Kian-Tat Lim added a comment - Two small comments on the PR, one on the code and one on the documentation.
            Hide
            rowen Russell Owen added a comment -

            Thank you for the very helpful corrections. I have corrected both issues and will merge as soon as it passes Jenkins.

            Show
            rowen Russell Owen added a comment - Thank you for the very helpful corrections. I have corrected both issues and will merge as soon as it passes Jenkins.
            Hide
            rowen Russell Owen added a comment -

            I found and fixed a bug in the short circuit code. The new code should also be somewhat more efficient, since it only records measured error if the error is too large. Merged to master.

            Show
            rowen Russell Owen added a comment - I found and fixed a bug in the short circuit code. The new code should also be somewhat more efficient, since it only records measured error if the error is too large. Merged to master.

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Kian-Tat Lim
              Watchers:
              Kian-Tat Lim, Russell Owen, Scott Daniel, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: