Details
-
Type:
Bug
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: pipe_tasks
-
Labels:None
-
Story Points:1
-
Epic Link:
-
Team:Data Release Production
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.7579544029403025In [5]: random.seed(0)In [6]: random.random()Out[6]: 0.8444218515250481In [7]: random.seed(0)In [8]: random.random()Out[8]: 0.8444218515250481In [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
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.
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. Some final comments on the PR. My above comments about commit structure still stand, but my review is complete.
Also, please don't forget to rebase to master and run Jenkins before merging.
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.
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. 
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.