Details

    • Type: Story
    • Status: Done
    • Priority: Major
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None
    • Templates:
    • Story Points:
      1
    • Team:
      Architecture

      Description

      Work required to allow AFW to work on Python 3.

        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 added a comment -

          Looks good. Some minor issues on the pull request.

          Show
          pschella Pim Schellart 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
              Watchers:
              Hsin-Fang Chiang, Jim Bosch, Pim Schellart, Russell Owen, Tim Jenness
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development