# broken code in meas_algorithms utils

XMLWordPrintable

## Details

• Type: Bug
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
0.1
• Sprint:
Science Pipelines DM-W16-2
• Team:
Data Release Production

## Description

meas.algorithms.utils has some broken code, including:

namp is not defined here (or anywhere) in showPsfResiduals:

  if showAmps:  nx, ny = namp 

dmin may be unassigned in showPsfCandidates:

  if len(catalog) == 1:  source = catalog[0]  else: # more than one source; find the once closest to (xc, yc)  for i, s in enumerate(catalog):  d = numpy.hypot(xc - s.getX(), yc - s.getY())  if i == 0 or d < dmin:  source, dmin = s, d 

Furthermore many variables are assigned and then never used. Try a linter such as pyflakes.

## Activity

Hide
John Swinbank added a comment -

Thanks Russell; I'm happy with your change – are you with mine? If so, I propose to merge them, regard the immediate problem as solved, close this ticket, and open another one to note that we should investigate the unused variables some rainy day.

Show
John Swinbank added a comment - Thanks Russell; I'm happy with your change – are you with mine? If so, I propose to merge them, regard the immediate problem as solved, close this ticket, and open another one to note that we should investigate the unused variables some rainy day.
Hide
Russell Owen added a comment -

Yes. John Swinbank that sounds perfect.

Show
Russell Owen added a comment - Yes. John Swinbank that sounds perfect.
Hide
Robert Lupton added a comment -

I must say that I disagree about fragility. This code fragment clearly says that the first time through is special. If you initialise dmin you have to set a value (and 0 is wrong although it doesn't matter as dmin is in fact safely initialised). So you end up with

 dmin = 1e99 # set to stop the python linter from complaining that dmin isn't set; overwritten the first time through the loop 

Adding a comment to the i == 0 test is reasonable.

Show
Robert Lupton added a comment - I must say that I disagree about fragility. This code fragment clearly says that the first time through is special. If you initialise dmin you have to set a value (and 0 is wrong although it doesn't matter as dmin is in fact safely initialised). So you end up with dmin = 1e99 # set to stop the python linter from complaining that dmin isn't set; overwritten the first time through the loop Adding a comment to the i == 0 test is reasonable.
Hide
John Swinbank added a comment -

Russell's change (in which he initializes dmin to None) doesn't do any real harm: it will still get set to a more useful value the first time around the loop, just as before. If this makes his life easier (and enables us to move on from this issue!) I think it's fine.

Show
John Swinbank added a comment - Russell's change (in which he initializes dmin to None ) doesn't do any real harm: it will still get set to a more useful value the first time around the loop, just as before. If this makes his life easier (and enables us to move on from this issue!) I think it's fine.
Hide
Russell Owen added a comment -

I felt that using an invalid initial value for dmin was the safest thing to do. It should never be used, but if some future change to the logic accidentally gets it used, then it will raise a reasonable exception. Normally I would code such a loop by testing for that invalid value instead of testing the loop index, but the existing logic was fine and changing it seemed needlessly dangerous. It's a very innocuous change and makes pyflakes happy.

Show
Russell Owen added a comment - I felt that using an invalid initial value for dmin was the safest thing to do. It should never be used, but if some future change to the logic accidentally gets it used, then it will raise a reasonable exception. Normally I would code such a loop by testing for that invalid value instead of testing the loop index, but the existing logic was fine and changing it seemed needlessly dangerous. It's a very innocuous change and makes pyflakes happy.

## People

• Assignee:
John Swinbank
Reporter:
Russell Owen
Reviewers:
Russell Owen
Watchers:
John Swinbank, Robert Lupton, Russell Owen