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

broken code in meas_algorithms utils

    Details

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

      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.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank 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
            swinbank 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
            rowen Russell Owen added a comment -

            Yes. John Swinbank that sounds perfect.

            Show
            rowen Russell Owen added a comment - Yes. John Swinbank that sounds perfect.
            Hide
            rhl 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
            rhl 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
            swinbank 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
            swinbank 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
            rowen 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
            rowen 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:
                swinbank John Swinbank
                Reporter:
                rowen Russell Owen
                Reviewers:
                Russell Owen
                Watchers:
                John Swinbank, Robert Lupton, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel