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

Fix order of arguments - run method of meas_base SingleFrameMeasurementTask

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:
      None

      Description

      In sfm.py on line 271, a comment indicates that some code is a temporary work around until the switch from meas_algorithms to meas_base is complete. This work is complete, so this temporary workaround should be removed, or if it is decided it should be kept, the comment should be removed. See https://github.com/lsst/meas_base/blob/tickets/DM-2915/python/lsst/meas/base/sfm.py#L271

        Attachments

          Issue Links

            Activity

            Hide
            nlust Nate Lust added a comment -

            Upon looking at this further, It probably is a serious thing to look into and fix, as it is swapping the variable names of arguments passed to the function. It would be a bad situation if two "orders" of calling the functions existed in the stack. Or worse still if everyone just "learned" that the arguments must be flipped, and the documentation indicated the wrong behavior.

            Show
            nlust Nate Lust added a comment - Upon looking at this further, It probably is a serious thing to look into and fix, as it is swapping the variable names of arguments passed to the function. It would be a bad situation if two "orders" of calling the functions existed in the stack. Or worse still if everyone just "learned" that the arguments must be flipped, and the documentation indicated the wrong behavior.
            Hide
            swinbank John Swinbank added a comment -

            Needs an RFC before this goes ahead.

            Show
            swinbank John Swinbank added a comment - Needs an RFC before this goes ahead.
            Hide
            pgee Perry Gee added a comment -

            Pulled your name out of a hat. But is change is extremely small.

            Show
            pgee Perry Gee added a comment - Pulled your name out of a hat. But is change is extremely small.
            Hide
            swinbank John Swinbank added a comment -

            Note that Merlin Fisher-Levine is travelling for the rest of this week — not sure when he'll be able to get to this review. If you don't mind waiting a few days, it should be fine.

            Show
            swinbank John Swinbank added a comment - Note that Merlin Fisher-Levine is travelling for the rest of this week — not sure when he'll be able to get to this review. If you don't mind waiting a few days, it should be fine.
            Hide
            swinbank John Swinbank added a comment -

            NB Merlin has asked me to take over this review.

            Show
            swinbank John Swinbank added a comment - NB Merlin has asked me to take over this review.
            Hide
            swinbank John Swinbank added a comment -

            Changes to the code look good – thank you Perry Gee!

            Some minor housekeeping issues to clear up:

            • Some commit messages are malformed. Remember that the first line should be no more than 50 characters. Please fix here, here and here.
            • Please make sure the e-mail address on your commits is associated with your GitHub account. You seem to be using a mix of pgee@physics.ucdavis.com and pgee@physics.ucdavis.edu (I assume both are valid?) — register them with GitHub here.

            Other than that, assuming that you've tested these changes with Jenkins, feel free to go ahead and merge.

            Show
            swinbank John Swinbank added a comment - Changes to the code look good – thank you Perry Gee ! Some minor housekeeping issues to clear up: Some commit messages are malformed. Remember that the first line should be no more than 50 characters. Please fix here , here and here . Please make sure the e-mail address on your commits is associated with your GitHub account. You seem to be using a mix of pgee@physics.ucdavis.com and pgee@physics.ucdavis.edu (I assume both are valid?) — register them with GitHub here . Other than that, assuming that you've tested these changes with Jenkins, feel free to go ahead and merge.
            Hide
            pgee Perry Gee added a comment -

            John Swinbank, I was hoping to develop contacts with some of the new DM staff so that getting reviews done would become easier. For meas_base and meas_algorithms, do you have a suggestion (among people who have been onboard for a year or so)?

            Show
            pgee Perry Gee added a comment - John Swinbank , I was hoping to develop contacts with some of the new DM staff so that getting reviews done would become easier. For meas_base and meas_algorithms, do you have a suggestion (among people who have been onboard for a year or so)?
            Hide
            swinbank John Swinbank added a comment -

            Well, I don't think you need any further review on this ticket, so it's not a problem here.

            However, for DM-9249, maybe Bob Armstrong would be a likely candidate?

            Show
            swinbank John Swinbank added a comment - Well, I don't think you need any further review on this ticket, so it's not a problem here. However, for DM-9249 , maybe Bob Armstrong would be a likely candidate?

              People

              Assignee:
              pgee Perry Gee
              Reporter:
              nlust Nate Lust
              Reviewers:
              John Swinbank
              Watchers:
              John Swinbank, Merlin Fisher-Levine, Nate Lust, Perry Gee
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: