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

Port pipe_tasks to Python 3

    XMLWordPrintable

Details

    • 3
    • Alert Production F16 - 9, Alert Production F16 - 10, Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c
    • Alert Production

    Attachments

      Issue Links

        Activity

          lauren I had to change the tolerance on the testProcessCcd tests in pipe_tasks. It appears that the previous restrictions were too tight but can you verify that the new values I've chosen fall within acceptable limits? See https://github.com/lsst/pipe_tasks/pull/77/commits/9daa645ad793354a20849348fef3720909006b34

          fred3m Fred Moolekamp added a comment - lauren I had to change the tolerance on the testProcessCcd tests in pipe_tasks. It appears that the previous restrictions were too tight but can you verify that the new values I've chosen fall within acceptable limits? See https://github.com/lsst/pipe_tasks/pull/77/commits/9daa645ad793354a20849348fef3720909006b34

          All of the tests pass with the current parameter values but per tjenness and rowen the setL method in afw is broken, which truncates visit ID's to 32 bits. This will need to be fixed before this ticket can be completed.

          fred3m Fred Moolekamp added a comment - All of the tests pass with the current parameter values but per tjenness and rowen the setL method in afw is broken, which truncates visit ID's to 32 bits. This will need to be fixed before this ticket can be completed.

          I didn't change the tolerances, just the values, so it's not obvious why the tolerances are now too stringent (and by so much!). Did the test pass on whatever compiler you're using in a pre-DM-6815 state? Is it possible your stack is not entirely in sync with master (I doubt it, but it's worth a double check)??

          These tolerances were set here by rowen, so he may have some opinion as to how tight they ought to be (although he does say in the commit message that the choice was arbitrary).

          lauren Lauren MacArthur added a comment - I didn't change the tolerances, just the values, so it's not obvious why the tolerances are now too stringent (and by so much!). Did the test pass on whatever compiler you're using in a pre- DM-6815 state? Is it possible your stack is not entirely in sync with master (I doubt it, but it's worth a double check)?? These tolerances were set here by rowen , so he may have some opinion as to how tight they ought to be (although he does say in the commit message that the choice was arbitrary).

          The difference is the python 3 vs python 2 stack. The python 3 stack was just rebuilt today, so it is definitely up to date, and I rebased my working version of pipe_tasks. Unless we believe this is a bug in the code running in python 3 the best thing to do might be to open up a ticket to review the requirements of the test and leave the current tolerances for the python 3 port. tjenness, swinbank, does that sound reasonable?

          fred3m Fred Moolekamp added a comment - The difference is the python 3 vs python 2 stack. The python 3 stack was just rebuilt today, so it is definitely up to date, and I rebased my working version of pipe_tasks. Unless we believe this is a bug in the code running in python 3 the best thing to do might be to open up a ticket to review the requirements of the test and leave the current tolerances for the python 3 port. tjenness , swinbank , does that sound reasonable?

          Those changes look very substantial to me: I'm not comfortable with them without more explanation. Correct me if I'm wrong, but I don't think we've seen switching to Python 3 making a big change to our numerics elsewhere.

          We do know we get numerical differences between Mac and Linux (although they're quite small). Have you checked this test on both platforms?

          swinbank John Swinbank added a comment - Those changes look very substantial to me: I'm not comfortable with them without more explanation. Correct me if I'm wrong, but I don't think we've seen switching to Python 3 making a big change to our numerics elsewhere. We do know we get numerical differences between Mac and Linux (although they're quite small). Have you checked this test on both platforms?

          (After discussion with krughoff, setting this back to the AP team and letting Fred off the hook!)

          swinbank John Swinbank added a comment - (After discussion with krughoff , setting this back to the AP team and letting Fred off the hook!)
          mrawls Meredith Rawls added a comment - - edited

          Thanks to some sleuthing by Parejkoj, we discovered that python's random number generator spits out different values in py2 and py3, which is likely behind this discrepancy. Running this simple code (from this site) yields different results in our py2 and py3.

          import random
          random.seed(0)
          for x in range(0, 10):
              random.randint(0, 1000000)
          

          py2 result:
          844422
          757955
          420572
          258917
          511275
          404934
          783799
          303313
          476597
          583382

          py3 result:
          885440
          403958
          794772
          933488
          441001
          42450
          271493
          536110
          509532
          424604

          However, doing the same thing with from numpy import random gives the same set of values in both cases:
          985772
          305711
          435829
          117952
          963395
          152315
          882371
          359783
          304137
          122579

          mrawls Meredith Rawls added a comment - - edited Thanks to some sleuthing by Parejkoj , we discovered that python's random number generator spits out different values in py2 and py3, which is likely behind this discrepancy. Running this simple code ( from this site ) yields different results in our py2 and py3. import random random.seed(0) for x in range(0, 10): random.randint(0, 1000000) py2 result: 844422 757955 420572 258917 511275 404934 783799 303313 476597 583382 py3 result: 885440 403958 794772 933488 441001 42450 271493 536110 509532 424604 However, doing the same thing with from numpy import random gives the same set of values in both cases: 985772 305711 435829 117952 963395 152315 882371 359783 304137 122579

          Unfortunately, it appears that changing import random to from numpy import random in both measurePsf and imageDifference has no change on what testProcessCcd.py spits out in either python version. (I also took care to change random.sample() -> numpy.random.choice().)

          mrawls Meredith Rawls added a comment - Unfortunately, it appears that changing import random to from numpy import random in both measurePsf and imageDifference has no change on what testProcessCcd.py spits out in either python version. (I also took care to change random.sample() -> numpy.random.choice() .)

          The aforementioned random was not the underlying issue here at all - the numerical differences are being caused by a difference in the background model. This is outside the scope of this ticket, so I'm going to wrap it up and open new tickets pertaining to meas_algorithms. A full writeup of what I found is here on Community. My only changes to the code wound up being formatting tweaks, including DM-7769.

          mrawls Meredith Rawls added a comment - The aforementioned random was not the underlying issue here at all - the numerical differences are being caused by a difference in the background model. This is outside the scope of this ticket, so I'm going to wrap it up and open new tickets pertaining to meas_algorithms . A full writeup of what I found is here on Community . My only changes to the code wound up being formatting tweaks, including DM-7769 .
          mrawls Meredith Rawls added a comment - - edited

          Your name came up as a possible reviewer for this, nlust. Please let me know if you can't do it - thanks!

          mrawls Meredith Rawls added a comment - - edited Your name came up as a possible reviewer for this, nlust . Please let me know if you can't do it - thanks!
          nlust Nate Lust added a comment -

          I was away last week, and am just getting back to the swing of things. We have a planning meeting today, but I will get to this in the next day or two. Does this time frame work for you?

          nlust Nate Lust added a comment - I was away last week, and am just getting back to the swing of things. We have a planning meeting today, but I will get to this in the next day or two. Does this time frame work for you?

          That's fine. I think it should be a quick review. Thank you!

          mrawls Meredith Rawls added a comment - That's fine. I think it should be a quick review. Thank you!
          nlust Nate Lust added a comment -

          Just a few minor points, but I think they might be addressed in a ticket you mentioned. Please make sure they are covered in work somewhere, and that this passes jenkins and is rebased up to current. Then you are good to merge.

          nlust Nate Lust added a comment - Just a few minor points, but I think they might be addressed in a ticket you mentioned. Please make sure they are covered in work somewhere, and that this passes jenkins and is rebased up to current. Then you are good to merge.

          Thanks, yes, I opened new tickets because there's a deeper py2/py3 background mismatch beyond the scope of this py3 port.

          It turns out the py2 Jenkins is failing because of a string issue that I missed before. (I must have only checked it with py3 Jenkins.) I traced the problem to something in matchBackground that uses listField from pex_config. I'll let you know once I fix it so py2 Jenkins passes and ask you to please take a final look.

          mrawls Meredith Rawls added a comment - Thanks, yes, I opened new tickets because there's a deeper py2/py3 background mismatch beyond the scope of this py3 port. It turns out the py2 Jenkins is failing because of a string issue that I missed before. (I must have only checked it with py3 Jenkins.) I traced the problem to something in matchBackground that uses listField from pex_config . I'll let you know once I fix it so py2 Jenkins passes and ask you to please take a final look.

          I've gotten pipe_tasks and pex_config to pass py2 Jenkins by making a tickets/DM-7292 branch for pex_config and editing pex_config/listField per tjenness' suggestion. However, the full build I ran failed on obs_subaru. Something I didn't touch, pex_config/dictField, is apparently causing a FieldValidationError when obs_subaru is tested. I can't tell if this problem arose indirectly from my small edit to pex_config/listField or not? It's strings all the way down. Here is the relevant Jenkins brain dump. Input appreciated.

          mrawls Meredith Rawls added a comment - I've gotten pipe_tasks and pex_config to pass py2 Jenkins by making a tickets/ DM-7292 branch for pex_config and editing pex_config/listField per tjenness ' suggestion. However, the full build I ran failed on obs_subaru . Something I didn't touch, pex_config/dictField , is apparently causing a FieldValidationError when obs_subaru is tested. I can't tell if this problem arose indirectly from my small edit to pex_config/listField or not? It's strings all the way down. Here is the relevant Jenkins brain dump. Input appreciated.
          tjenness Tim Jenness added a comment -

          My guess is that listField needs the same str translation fix.

          tjenness Tim Jenness added a comment - My guess is that listField needs the same str translation fix.

          I did put your str translation fix in listField, but it looks to me like dictField is written differently and doesn't explicitly use {{dtype}}s.

          mrawls Meredith Rawls added a comment - I did put your str translation fix in listField , but it looks to me like dictField is written differently and doesn't explicitly use {{dtype}}s.

          This went much deeper than it needed to... turns out there were still a couple from builtins import str lines in pipe_tasks that shouldn't have been there. They're gone now, and Jenkins is happy on py2 and py3! The tickets/DM-7292 branch of pex_config is identical to master and may be ignored. nlust, please confirm you are OK with me going ahead and merging.

          mrawls Meredith Rawls added a comment - This went much deeper than it needed to... turns out there were still a couple from builtins import str lines in pipe_tasks that shouldn't have been there. They're gone now, and Jenkins is happy on py2 and py3! The tickets/ DM-7292 branch of pex_config is identical to master and may be ignored. nlust , please confirm you are OK with me going ahead and merging.

          Discussed with Nate in Slack, fixed up a couple last tiny things, all good!

          mrawls Meredith Rawls added a comment - Discussed with Nate in Slack, fixed up a couple last tiny things, all good!

          People

            mrawls Meredith Rawls
            tjenness Tim Jenness
            Nate Lust
            Fred Moolekamp, John Parejko, John Swinbank, Lauren MacArthur, Meredith Rawls, Nate Lust, Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Jenkins

                No builds found.