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

str(Image) tests too strict about formatting

    Details

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

      Description

      While the changes introduced in DM-15682 passed formal testing, I discovered that they do not pass if afw is built against NumPy 1.13 – that version of numpy includes more whitespace in the array than the test expects:

      AssertionError: '[[0. 0.]\n [0. 0.]]' not found in '[[ 0.  0.]\n [ 0.  0.]], bbox=(minimum=(0, 0), maximum=(1, 1))'
      

      While the build failure can be worked around by upgrading NumPy to 1.14 or later (which will soon be required anyway), the consensus on #dm-build-problems was that any future formatting changes in NumPy would break the test again.

      Please modify the tests so that they're less sensitive to NumPy's formatting decisions, preferably while keeping some diagnostic of whether the array is included in the string.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            The problem here arises because these tests conflate two questions: “are we correctly delegating to NumPy stringification?” and “does NumPy stringification look like we expect it to?”. I suggest that the relevant question is really the former.

            Krzysztof Findeisen, would you mind taking a look at my changes? I hope this will work on NumPy 1.13 but I don't have a convenient system to check on.

            Show
            swinbank John Swinbank added a comment - The problem here arises because these tests conflate two questions: “are we correctly delegating to NumPy stringification?” and “does NumPy stringification look like we expect it to?”. I suggest that the relevant question is really the former. Krzysztof Findeisen , would you mind taking a look at my changes? I hope this will work on NumPy 1.13 but I don't have a convenient system to check on.
            Hide
            swinbank John Swinbank added a comment -

            Since Jira hasn't picked it up yet, I'll add that the PR is here: https://github.com/lsst/afw/pull/413

            Show
            swinbank John Swinbank added a comment - Since Jira hasn't picked it up yet, I'll add that the PR is here: https://github.com/lsst/afw/pull/413
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            I disagree. Delegating to NumPy is an implementation detail; what we really should be testing is "does the string contain a reasonable array-like representation of the image pixels?".

            That said, the original version of the test also assumed a NumPy implementation, if less explicitly, and this approach does solve the forward-compatibility problem.

            Show
            krzys Krzysztof Findeisen added a comment - - edited I disagree. Delegating to NumPy is an implementation detail; what we really should be testing is "does the string contain a reasonable array-like representation of the image pixels?". That said, the original version of the test also assumed a NumPy implementation, if less explicitly, and this approach does solve the forward-compatibility problem.
            Hide
            swinbank John Swinbank added a comment -

            Thanks for the review!

            I agree that one could imagine a test for “a reasonable representation”, but I think then we'd need to bring the implementation in-house: delegating generation of output to NumPy while testing against our own internal standard seems doomed to hopeless fragility.

            Anyway, this will do for this ticket; merged and done.

            Show
            swinbank John Swinbank added a comment - Thanks for the review! I agree that one could imagine a test for “a reasonable representation”, but I think then we'd need to bring the implementation in-house: delegating generation of output to NumPy while testing against our own internal standard seems doomed to hopeless fragility. Anyway, this will do for this ticket; merged and done.
            Hide
            Parejkoj John Parejko added a comment -

            I think this is a good approach.

            Show
            Parejkoj John Parejko added a comment - I think this is a good approach.

              People

              • Assignee:
                swinbank John Swinbank
                Reporter:
                krzys Krzysztof Findeisen
                Reviewers:
                Krzysztof Findeisen
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Krzysztof Findeisen
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel