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

testExposure.testGetWcs docstring is wrong, and tests should be assertIsNone

    XMLWordPrintable

    Details

      Description

      The docstring for testExposure.testGetWcs does not match the implemented tests in the method, and at least some of those tests appear to be incorrect, given the current (SWIGed) behavior of lsst.afw.image.Wcs. I've listed several obvious problems below:

      • The docstring claims exceptions should be raised, but none of the tests check for exceptions.
      • In addition, exposure.getWcs() returns None if the exposure was initialized without a Wcs, not False. None is "falsey", but if the API really wants None returned, we should test for that explicitly.
      • The two unadorned getWcs() calls should have an assertEqual against self.wcs, since that's what those exposures were initialized with.
      • I also noticed testSetMembers, which catches a pex.Exception, prints a message, and then continues on its merry way. This should also be fixed.

      This whole test suite needs to be looked at. That might be beyond scope for this ticket, but all of the above points are very worrying.

        Attachments

          Activity

          Hide
          rowen Russell Owen added a comment - - edited

          Good catch. One minor point: WCS do not have a useful equality operator. If the IDs match then that's an easy test. Otherwise one can compare two WCS over a region, e.g. using assertWcsNearlyEqualOverBBox.

          Show
          rowen Russell Owen added a comment - - edited Good catch. One minor point: WCS do not have a useful equality operator. If the IDs match then that's an easy test. Otherwise one can compare two WCS over a region, e.g. using assertWcsNearlyEqualOverBBox .
          Hide
          swinbank John Swinbank added a comment - - edited

          These changes look good to me; let's not block on waiting for Nate to have time to review them.

          One minor tweak: could you please make the summary sentence in the docstring start on the same line as the """? Then, good to merge assuming Jenkins is happy.

          Show
          swinbank John Swinbank added a comment - - edited These changes look good to me; let's not block on waiting for Nate to have time to review them. One minor tweak: could you please make the summary sentence in the docstring start on the same line as the """ ? Then, good to merge assuming Jenkins is happy.

            People

            Assignee:
            mrawls Meredith Rawls
            Reporter:
            Parejkoj John Parejko
            Reviewers:
            John Swinbank
            Watchers:
            John Parejko, John Swinbank, Nate Lust, Pim Schellart [X] (Inactive), Russell Owen, Simon Krughoff
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.