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

Port afw to Python 3

    Details

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

      Description

      Work required to allow AFW to work on Python 3.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            Port complete. All tests pass on Jenkins on Python 2 (including lsst_ci). Python 3 passes all tests except for a test in random.py where setState crashes because the bytes from getState are being interpreted as a unicode string.

            Show
            tjenness Tim Jenness added a comment - Port complete. All tests pass on Jenkins on Python 2 (including lsst_ci ). Python 3 passes all tests except for a test in random.py where setState crashes because the bytes from getState are being interpreted as a unicode string.
            Hide
            tjenness Tim Jenness added a comment -

            Can you please review this (you seem to already be looking at the pull request)?

            Show
            tjenness Tim Jenness added a comment - Can you please review this (you seem to already be looking at the pull request)?
            Hide
            jbosch Jim Bosch added a comment -

            I have a proposed fix for the Random::getState/setState issue on u/jbosch/DM-7152. I have not had a chance to test it, and likely will not before next week, but it should at least provide a skeleton for a solution. One thing that definitely needs to be tested: what happens when something other than bytes is passed to setState; the Python C API code I've written will correctly raise TypeError, but I'm not sure if Swig will call PyErr_Occurred and return null to finish the error propagation when that happens.

            Show
            jbosch Jim Bosch added a comment - I have a proposed fix for the Random::getState/setState issue on u/jbosch/ DM-7152 . I have not had a chance to test it, and likely will not before next week, but it should at least provide a skeleton for a solution. One thing that definitely needs to be tested: what happens when something other than bytes is passed to setState ; the Python C API code I've written will correctly raise TypeError , but I'm not sure if Swig will call PyErr_Occurred and return null to finish the error propagation when that happens.
            Hide
            tjenness Tim Jenness added a comment -

            Jim Bosch works great and tests pass. If I try to pass in a string instead of bytes you guessed the outcome:

            TypeError: expected bytes, str found
             
            During handling of the above exception, another exception occurred:
             
            Traceback (most recent call last):
              File "tests/random1.py", line 121, in testState
                self.rand.setState("hello")
              File "/Users/timj/work/lsstsw3/build/afw/python/lsst/afw/math/mathLib.py", line 8425, in setState
                return _mathLib.Random_setState(self, state)
            SystemError: <built-in function Random_setState> returned a result with an error set
            

            Less than ideal but I doubt it matters much in the short term.

            Show
            tjenness Tim Jenness added a comment - Jim Bosch works great and tests pass. If I try to pass in a string instead of bytes you guessed the outcome: TypeError: expected bytes, str found   During handling of the above exception, another exception occurred:   Traceback (most recent call last): File "tests/random1.py", line 121, in testState self.rand.setState("hello") File "/Users/timj/work/lsstsw3/build/afw/python/lsst/afw/math/mathLib.py", line 8425, in setState return _mathLib.Random_setState(self, state) SystemError: <built-in function Random_setState> returned a result with an error set Less than ideal but I doubt it matters much in the short term.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Looks good. Some minor issues on the pull request.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Looks good. Some minor issues on the pull request.
            Hide
            tjenness Tim Jenness added a comment -

            Thanks. I've pushed some fixes for your comments. I'm not going to attempt to fix the _future_ issues in this commit. I am running a Jenkins build now. I probably should squash some of the fixes that I have just done and then I'll try to merge tomorrow before the afternoon session.

            Show
            tjenness Tim Jenness added a comment - Thanks. I've pushed some fixes for your comments. I'm not going to attempt to fix the _ future _ issues in this commit. I am running a Jenkins build now. I probably should squash some of the fixes that I have just done and then I'll try to merge tomorrow before the afternoon session.
            Hide
            rowen Russell Owen added a comment -

            Tim Jenness discovered an intermittent problem in tests/testCameraGeom.py in the testTransformDet method: it would fail 1 in 5 or 10 times due to the order of dict keys being returned. The obvious fix that Tim Jenness suggested is to test all detectors instead of a subset. I adopted that fix and added some comments to explain more of what is going on. I think the test could use additional improvements (e.g. a more deterministic decision if a point off one detector should be found on another detector), but I think the current test is adequate (with this fix).

            Show
            rowen Russell Owen added a comment - Tim Jenness discovered an intermittent problem in tests/testCameraGeom.py in the testTransformDet method: it would fail 1 in 5 or 10 times due to the order of dict keys being returned. The obvious fix that Tim Jenness suggested is to test all detectors instead of a subset. I adopted that fix and added some comments to explain more of what is going on. I think the test could use additional improvements (e.g. a more deterministic decision if a point off one detector should be found on another detector), but I think the current test is adequate (with this fix).
            Hide
            tjenness Tim Jenness added a comment -

            Thanks to everyone for helping me out on this. Everything passes. Merged!

            Show
            tjenness Tim Jenness added a comment - Thanks to everyone for helping me out on this. Everything passes. Merged!

              People

              • Assignee:
                tjenness Tim Jenness
                Reporter:
                tjenness Tim Jenness
                Reviewers:
                Pim Schellart [X] (Inactive)
                Watchers:
                Hsin-Fang Chiang, Jim Bosch, Pim Schellart [X] (Inactive), Russell Owen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel