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

DipoleClassificationConfig.setDefaults returns before doing anything

    XMLWordPrintable

Details

    • Bug
    • Status: Done
    • Resolution: Done
    • None
    • ip_diffim
    • None

    Description

      lsst.ip.diffim.dipoleMeasurement.DipoleClassificationConfig.setDefaults has a return before the business logic. That can't be right.

      Attachments

        Issue Links

          Activity

            No builds found.
            swinbank John Swinbank created issue -

            krughoff I assigned this to the AP team since it's in diffim. I'm guessing the fix is basically trivial, but I've not yet investigated to see whether the return is a mistake and should be removed or if the whole method is unnecessary (I'm guessing the latter since everything seems to work and the tests pass as is).

            swinbank John Swinbank added a comment - krughoff I assigned this to the AP team since it's in diffim. I'm guessing the fix is basically trivial, but I've not yet investigated to see whether the return is a mistake and should be removed or if the whole method is unnecessary (I'm guessing the latter since everything seems to work and the tests pass as is).

            Looks like this was put in by Perry on DM-980. Might want to ping him on
            this as well.
            Andy

            On Thu, Jul 16, 2015 at 4:16 PM, John Swinbank (JIRA) <jira-dm@lsst.org>

            robyn Robyn Allsman [X] (Inactive) added a comment - Looks like this was put in by Perry on DM-980 . Might want to ping him on this as well. Andy On Thu, Jul 16, 2015 at 4:16 PM, John Swinbank (JIRA) <jira-dm@lsst.org>

            Thanks; I suspect this just got garbled in the transition to the meas_base framework, but pgee please do chime in if you have anything to add!

            swinbank John Swinbank added a comment - Thanks; I suspect this just got garbled in the transition to the meas_base framework, but pgee please do chime in if you have anything to add!

            After a few minutes of poking about, I decided we do want to keep this method, but fix it up so it actually works properly and then make use of it. Fix is just a few lines.

            swinbank John Swinbank added a comment - After a few minutes of poking about, I decided we do want to keep this method, but fix it up so it actually works properly and then make use of it. Fix is just a few lines.
            swinbank John Swinbank made changes -
            Field Original Value New Value
            Assignee Andrew Becker [ abecker ] John Swinbank [ swinbank ]
            Status To Do [ 10001 ] In Progress [ 3 ]

            Ok, looks like tickets/DM-3159 on ip_diffim does the trick. pgee, could I trouble you for a quick review please?

            swinbank John Swinbank added a comment - Ok, looks like tickets/DM-3159 on ip_diffim does the trick. pgee , could I trouble you for a quick review please?
            swinbank John Swinbank made changes -
            Reviewers Perry Gee [ pgee ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            swinbank John Swinbank made changes -
            Epic Link DM-1912 [ 15945 ]
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-S15-5 [ 162 ]
            Story Points 0.1
            swinbank John Swinbank made changes -
            Team Alert Production [ 10300 ] Data Release Production [ 10301 ]
            pgee Perry Gee added a comment -

            The changes look OK. However, at least on my machine, a few of the unit tests are failing. Do these pass on buildBot? If so, I won't worry about it. They are specific failures, unrelated to the change in this ticket.

            BTW, I did not get this stream of questions in my email, or perhaps it is being spam filtered. I apologize for not responding.

            As far as the "return" is concerned, my recollection is that at the time, the setDefaults was not working correctly, though I only have a vague recollection of it. I don't think there was a super class setDefault() call in the original code, and perhaps there should have been. I was therefore forced to put these settings in other places to get the unit tests to pass.

            I checked it in in this weird state because Simon thought that the code needed rework, and that someone would be assigned to work on it when he got some new hires and could figure out the problems. I should have commented why the return was there at the time.

            pgee Perry Gee added a comment - The changes look OK. However, at least on my machine, a few of the unit tests are failing. Do these pass on buildBot? If so, I won't worry about it. They are specific failures, unrelated to the change in this ticket. BTW, I did not get this stream of questions in my email, or perhaps it is being spam filtered. I apologize for not responding. As far as the "return" is concerned, my recollection is that at the time, the setDefaults was not working correctly, though I only have a vague recollection of it. I don't think there was a super class setDefault() call in the original code, and perhaps there should have been. I was therefore forced to put these settings in other places to get the unit tests to pass. I checked it in in this weird state because Simon thought that the code needed rework, and that someone would be assigned to work on it when he got some new hires and could figure out the problems. I should have commented why the return was there at the time.
            pgee Perry Gee added a comment -

            Changes look fine, with the proviso that some of the unit tests are failing. See my previous comment.

            pgee Perry Gee added a comment - Changes look fine, with the proviso that some of the unit tests are failing. See my previous comment.
            pgee Perry Gee made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]

            Thanks pgee. I'm a bit worried about the failing tests, though. Since they work for me and for CI I think everything is fine, but perhaps you could provide bit more detail about what the problem is?

            Sorry to hear about your e-mail difficulties! It's always a tiresome when something like that breaks. However, we really do rely on people being responsive to JIRA in order to get things done. If the problem recurs, please do take time to track it down and make sure it's fixed – you'll find that frossie and the rest of the SQuaRE team can help with JIRA problems, but you'll need to take care of your own spam filters!

            swinbank John Swinbank added a comment - Thanks pgee . I'm a bit worried about the failing tests, though. Since they work for me and for CI I think everything is fine, but perhaps you could provide bit more detail about what the problem is? Sorry to hear about your e-mail difficulties! It's always a tiresome when something like that breaks. However, we really do rely on people being responsive to JIRA in order to get things done. If the problem recurs, please do take time to track it down and make sure it's fixed – you'll find that frossie and the rest of the SQuaRE team can help with JIRA problems, but you'll need to take care of your own spam filters!

            I am going to do a complete new release, and assuming it builds, I will try
            again. May be that something is out of date.

            I do think there is something wrong with my email notifications, so I am
            going to try to learn more about it and see if I can figure it out.

            On Thu, Jul 30, 2015 at 3:32 PM, John Swinbank (JIRA) <jira-dm@lsst.org>

            robyn Robyn Allsman [X] (Inactive) added a comment - I am going to do a complete new release, and assuming it builds, I will try again. May be that something is out of date. I do think there is something wrong with my email notifications, so I am going to try to learn more about it and see if I can figure it out. On Thu, Jul 30, 2015 at 3:32 PM, John Swinbank (JIRA) <jira-dm@lsst.org>
            tjenness Tim Jenness made changes -
            Link This issue relates to DM-3350 [ DM-3350 ]
            frossie Frossie Economou made changes -
            Assignee John Swinbank [ swinbank ] Frossie Economou [ frossie ]

            META: Will the person who made the 2nd and 9th comment in this ticket please identify themselves to me (off-ticket) so we can figure out why JIRA is confused about your identity.

            frossie Frossie Economou added a comment - META: Will the person who made the 2nd and 9th comment in this ticket please identify themselves to me (off-ticket) so we can figure out why JIRA is confused about your identity.
            tjenness Tim Jenness made changes -
            Assignee Frossie Economou [ frossie ] John Swinbank [ swinbank ]
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-S15-5 [ 162 ] Science Pipelines DM-S15-5, Science Pipelines DM-S15-6 [ 162, 165 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher

            I made one of these comments. I think that this is an old problem which I
            thought we had fixed. It has to do with my email address not being known.
            I am using pgee2000@gmail.com,t but it is possible that I am listed as
            pgee@physics.ucdavis.edu in some places.

            On Thu, Jul 30, 2015 at 11:50 PM, Frossie Economou (JIRA) <jira-dm@lsst.org>

            robyn Robyn Allsman [X] (Inactive) added a comment - I made one of these comments. I think that this is an old problem which I thought we had fixed. It has to do with my email address not being known. I am using pgee2000@gmail.com,t but it is possible that I am listed as pgee@physics.ucdavis.edu in some places. On Thu, Jul 30, 2015 at 11:50 PM, Frossie Economou (JIRA) <jira-dm@lsst.org>

            pgee – any progress on this? It would be nice to close it before the end of this sprint/cycle.

            swinbank John Swinbank added a comment - pgee – any progress on this? It would be nice to close it before the end of this sprint/cycle.
            pgee Perry Gee added a comment -

            My intention was to OK this for checkup, subject to any unit tests which might be failing on buildBot. I have reviewed the code.

            pgee Perry Gee added a comment - My intention was to OK this for checkup, subject to any unit tests which might be failing on buildBot. I have reviewed the code.
            swinbank John Swinbank made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

            People

              swinbank John Swinbank
              swinbank John Swinbank
              Perry Gee
              Frossie Economou, John Swinbank, Perry Gee, Robyn Allsman [X] (Inactive), Simon Krughoff (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.