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

nondeterministic random number seeds in MeasurePsf candidate reservation

    Details

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

      Description

      MeasurePsfTask randomly reserves a fraction of its candidates for validation, in a way that is supposed to be deterministic. This seems to be broken; I've identified at least two problems:

      • CharacterizeImageTask does not pass the expId argument to MeasurePsfTask.run, letting it default to zero.
      • MeasurePsfTask uses Python's built-in random module instead of afw.math.Random. Contrary to its own documentation, calling random.seed(0) does not always produce deterministic results (though I've only been able to trigger this the first time I tried it):

        In [3]: random.seed(0)
         
        In [4]: random.random()
        Out[4]: 0.7579544029403025
         
        In [5]: random.seed(0)
         
        In [6]: random.random()
        Out[6]: 0.8444218515250481
         
        In [7]: random.seed(0)
         
        In [8]: random.random()
        Out[8]: 0.8444218515250481
         
        In [9]: random.seed(0)
         
        In [10]: random.random()
        Out[10]: 0.8444218515250481
        

      I have no idea what could be going on in the built-in random module (and it's hard to report upstream since I was only able to trigger it once), but we should switch to our own random number generator regardless.

        Attachments

          Issue Links

            Activity

            Hide
            pgee Perry Gee added a comment -

            I have been running a linter, but I did miss some things in the testPsfCandidateSelection.py. I cut-and-pasted this from the testPhotoCal, and guess I thought of the code as pre-existing when actually it was new.

            The unnecessary import in photoCal.py was there already, and I have been told not to fix such things as I am changing other code.
            I added this as an additional commit since you asked for it. I guess that is OK to do.

            I will fix the commit structure after you review the code as it currently stands. I know this can be done with rebase, but I need to figure out how.

            Show
            pgee Perry Gee added a comment - I have been running a linter, but I did miss some things in the testPsfCandidateSelection.py. I cut-and-pasted this from the testPhotoCal, and guess I thought of the code as pre-existing when actually it was new. The unnecessary import in photoCal.py was there already, and I have been told not to fix such things as I am changing other code. I added this as an additional commit since you asked for it. I guess that is OK to do. I will fix the commit structure after you review the code as it currently stands. I know this can be done with rebase, but I need to figure out how.
            Hide
            pgee Perry Gee added a comment -

            Lauren MacArthur I would like to finish up this ticket before I leave for good this week. I left it for you to finish the review when I took off for vacation in June, and it has been sitting every since, I guess.

            I rebased it yesterday, and tried to answer your objections, but some are pretty general, so I'm not sure.

            If you can't get to it, please let me know and I can ask John to take care of it.

            Show
            pgee Perry Gee added a comment - Lauren MacArthur I would like to finish up this ticket before I leave for good this week. I left it for you to finish the review when I took off for vacation in June, and it has been sitting every since, I guess. I rebased it yesterday, and tried to answer your objections, but some are pretty general, so I'm not sure. If you can't get to it, please let me know and I can ask John to take care of it.
            Hide
            lauren Lauren MacArthur added a comment -

            Some final comments on the PR. My above comments about commit structure still stand, but my review is complete.

            Show
            lauren Lauren MacArthur added a comment - Some final comments on the PR. My above comments about commit structure still stand, but my review is complete.
            Hide
            lauren Lauren MacArthur added a comment -

            Also, please don't forget to rebase to master and run Jenkins before merging.

            Show
            lauren Lauren MacArthur added a comment - Also, please don't forget to rebase to master and run Jenkins before merging.
            Hide
            pgee Perry Gee added a comment -

            I have fixed all the minor problems Lauren flagged which still appeared in the code. Paul has asked me in the past not to comment in github on every edit, but John has also told me that all problems should be either fixed or the reviewer should be told why not. So I have chosen to comment here.

            I fixed all the problems newly noted from Lauren's comments of 5 days ago, or those which she noted where still in the code from the previous review.

            I am now running Jenkins, and will merge when that is done.

            Show
            pgee Perry Gee added a comment - I have fixed all the minor problems Lauren flagged which still appeared in the code. Paul has asked me in the past not to comment in github on every edit, but John has also told me that all problems should be either fixed or the reviewer should be told why not. So I have chosen to comment here. I fixed all the problems newly noted from Lauren's comments of 5 days ago, or those which she noted where still in the code from the previous review. I am now running Jenkins, and will merge when that is done.

              People

              • Assignee:
                pgee Perry Gee
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                Lauren MacArthur
                Watchers:
                Jim Bosch, Lauren MacArthur, Perry Gee
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel