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

Post meas_base move changes to Kron

    XMLWordPrintable

    Details

      Description

      These are to note leftovers from DM-982. They could be done in a single issue.
      1. I commented code out referring to correctfluxes, but it will need to be restored once it is available in the new framework.

      2. Jim asked me to replace the computeSincFlux which is currently in PsfImage.cc in meas_algorithms with a similar call in meas_base/ApertureFlux.cc. I did not do this because it became rather complicated, and can just as easily be done when the meas_algorithms routine is moved or removed. Basically, the templating in ApertureFlux is on Pixel type, whereas in meas_algorithms it is on ImageT (where ImageT is not necessarily a single class hierarchy – e.g., Image and MaskedImage). So I left this for now.

        Attachments

          Issue Links

            Activity

            pgee Perry Gee created issue -
            swinbank John Swinbank made changes -
            Field Original Value New Value
            Link This issue relates to DM-420 [ DM-420 ]
            Hide
            swinbank John Swinbank added a comment -

            Do we have an issue for making correctfluxes code available in the new framework? Please make sure it's linked to this one, so this doesn't get forgotten.

            I assume the removal of computeSincFlux from meas_algorithms is covered by DM-420, so I've linked that issue. (I'm not sure I understand why deferring this work until that issue is addressed is desirable, but it doesn't seem to be a big deal).

            Jim Bosch Shall we add this to DM-1768 for S15?

            Show
            swinbank John Swinbank added a comment - Do we have an issue for making correctfluxes code available in the new framework? Please make sure it's linked to this one, so this doesn't get forgotten. I assume the removal of computeSincFlux from meas_algorithms is covered by DM-420 , so I've linked that issue. (I'm not sure I understand why deferring this work until that issue is addressed is desirable, but it doesn't seem to be a big deal). Jim Bosch Shall we add this to DM-1768 for S15?
            Hide
            jbosch Jim Bosch added a comment -

            "correctfluxes" was the old name for the code that applied aperture corrections, so the issue to replace it is epic DM-85. I'd rather remove that code here than comment it out, though (in general, we should never have commented-out code on master; if it's a block we think we might want to reenable, use #if 0, and if we just want to be able to see what the code was, we should use git).

            DM-420 is the correct issue for removal of calculate SincApertureFlux. I'm hoping we can get that done in the next sprint, but if we don't, it would land in DM-1768.

            Show
            jbosch Jim Bosch added a comment - "correctfluxes" was the old name for the code that applied aperture corrections, so the issue to replace it is epic DM-85 . I'd rather remove that code here than comment it out, though (in general, we should never have commented-out code on master; if it's a block we think we might want to reenable, use #if 0, and if we just want to be able to see what the code was, we should use git). DM-420 is the correct issue for removal of calculate SincApertureFlux. I'm hoping we can get that done in the next sprint, but if we don't, it would land in DM-1768.
            Hide
            pgee Perry Gee added a comment -

            I decided to defer the work because it was neither really part of the original ticket, nor did it seem to me wise to do it without some thought and consultation. I believe it would involve either rewriting the internals of the KronAlgorithm, or the routine in ApertureFlux. The problem is an inconsistency in the templating, between the two modules. We are more likely to get it right on a new ticket at the start of the next Sprint. I am happy to do the work then.

            Show
            pgee Perry Gee added a comment - I decided to defer the work because it was neither really part of the original ticket, nor did it seem to me wise to do it without some thought and consultation. I believe it would involve either rewriting the internals of the KronAlgorithm, or the routine in ApertureFlux. The problem is an inconsistency in the templating, between the two modules. We are more likely to get it right on a new ticket at the start of the next Sprint. I am happy to do the work then.
            Hide
            swinbank John Swinbank added a comment -

            I looked at the changes made in DM-982 regarding "correctfluxes". By my (superficial) reading, they look as if they're simply to prevent errors due to the algorithm no longer being available. If that's correct, then it looks to me as if that code can just vanish (and maybe the review of DM-982 will say that); when a replacement algorithm is implemented, if it causes similar errors, they'll naturally be fixed as part of adjusting the tests to handle that new algorithm.

            As for the sinc flux, it seems like it would have to be handled when the relevant routine is removed from meas_algorithms, otherwise it would cause obvious breakage. It doesn't seem like we need a separate story to track that.

            Given the above, I'm a bit minded to think there's nothing here that really requires a standalone story. Anybody object to my just shutting it down?

            Show
            swinbank John Swinbank added a comment - I looked at the changes made in DM-982 regarding "correctfluxes". By my (superficial) reading, they look as if they're simply to prevent errors due to the algorithm no longer being available. If that's correct, then it looks to me as if that code can just vanish (and maybe the review of DM-982 will say that); when a replacement algorithm is implemented, if it causes similar errors, they'll naturally be fixed as part of adjusting the tests to handle that new algorithm. As for the sinc flux, it seems like it would have to be handled when the relevant routine is removed from meas_algorithms, otherwise it would cause obvious breakage. It doesn't seem like we need a separate story to track that. Given the above, I'm a bit minded to think there's nothing here that really requires a standalone story. Anybody object to my just shutting it down?
            Hide
            robyn Robyn Allsman [X] (Inactive) added a comment -

            The code is there is comments as a convenience for the person who has to
            fix it when flux correction is implemented. It is not crucial that it be
            in the code, it is just a reminder. If that is frowned upon, I still would
            like to havean explicit reminder about this in some ticket, citing that the
            original Kron code in meas_extensions had it and that it was not ported.
            Since this code is in an extension, it could easily be forgotten.

            The only reason to have a ticket for the SincFlux stuff is if you feel like
            it should be done independently of the other work. If not, kill it.

            On Thu, Feb 5, 2015 at 1:41 PM, John Swinbank (JIRA) <jira-dm@lsst.org>

            Show
            robyn Robyn Allsman [X] (Inactive) added a comment - The code is there is comments as a convenience for the person who has to fix it when flux correction is implemented. It is not crucial that it be in the code, it is just a reminder. If that is frowned upon, I still would like to havean explicit reminder about this in some ticket, citing that the original Kron code in meas_extensions had it and that it was not ported. Since this code is in an extension, it could easily be forgotten. The only reason to have a ticket for the SincFlux stuff is if you feel like it should be done independently of the other work. If not, kill it. On Thu, Feb 5, 2015 at 1:41 PM, John Swinbank (JIRA) <jira-dm@lsst.org>
            swinbank John Swinbank made changes -
            Team Princeton [ 10301 ]
            swinbank John Swinbank made changes -
            Epic Link DM-1769 [ 15588 ]
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-S15-1 [ 140 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            pgee Perry Gee made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            pgee Perry Gee added a comment -

            My belief is that there is nothing left to do on this ticket. John, can you confirm?

            I removed the comments as part of an earlier review, and fixed the issue with moving the SincFlux calculation to the new ApertureFlux.cc code as part of DM-420.

            Show
            pgee Perry Gee added a comment - My belief is that there is nothing left to do on this ticket. John, can you confirm? I removed the comments as part of an earlier review, and fixed the issue with moving the SincFlux calculation to the new ApertureFlux.cc code as part of DM-420 .
            pgee Perry Gee made changes -
            Reviewers John Swinbank [ swinbank ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            swinbank John Swinbank added a comment - - edited

            As I recall, the comments in point 1 were there to remind us to add aperture corrections to meas_extensions_photometryKron when the relevant functionality was available. I think comments aren't a great way to do that, so I'm glad they're gone. But is it still important that photometryKron be updated when DM-85 is done? If so, either this issue or another needs to be tracking that.

            I agree that point 2 is addressed by DM-420.

            Show
            swinbank John Swinbank added a comment - - edited As I recall, the comments in point 1 were there to remind us to add aperture corrections to meas_extensions_photometryKron when the relevant functionality was available. I think comments aren't a great way to do that, so I'm glad they're gone. But is it still important that photometryKron be updated when DM-85 is done? If so, either this issue or another needs to be tracking that. I agree that point 2 is addressed by DM-420 .
            Hide
            pgee Perry Gee added a comment -

            John, can you move this issue to review complete or defer it, depending on whether you think that there is still work to be done on it? I think the only open issue has to do with aperture corrections.

            Show
            pgee Perry Gee added a comment - John, can you move this issue to review complete or defer it, depending on whether you think that there is still work to be done on it? I think the only open issue has to do with aperture corrections.
            Hide
            swinbank John Swinbank added a comment -

            Hi Perry – my comment above is still outstanding. If changes will need to be made to _photometryKron when DM-85 is done, we need a way of tracking that. It could be this issue, or it could be another issue, but it should be clear which. If those changes don't need to be made, then we can close this issue, but we need a clear statement to that effect.

            Show
            swinbank John Swinbank added a comment - Hi Perry – my comment above is still outstanding. If changes will need to be made to _photometryKron when DM-85 is done, we need a way of tracking that. It could be this issue, or it could be another issue, but it should be clear which. If those changes don't need to be made, then we can close this issue, but we need a clear statement to that effect.
            Hide
            pgee Perry Gee added a comment -

            i agree. So I will just take it out of the review loop and defer it.

            Show
            pgee Perry Gee added a comment - i agree. So I will just take it out of the review loop and defer it.
            Hide
            pgee Perry Gee added a comment -

            Actually, I guess there is no way for me to mark it as deferred, so will just put it is a working state again and you can move it to another sprint.

            Show
            pgee Perry Gee added a comment - Actually, I guess there is no way for me to mark it as deferred, so will just put it is a working state again and you can move it to another sprint.
            pgee Perry Gee made changes -
            Status In Review [ 10004 ] To Do [ 10001 ]
            Hide
            swinbank John Swinbank added a comment -

            Since this ticket covers multiple issues, I think it makes sense to close this one down now and file a specific ticket for the aperture corrections to _photometryKron. So I've done that; new ticket is DM-2429.

            Show
            swinbank John Swinbank added a comment - Since this ticket covers multiple issues, I think it makes sense to close this one down now and file a specific ticket for the aperture corrections to _photometryKron . So I've done that; new ticket is DM-2429 .
            swinbank John Swinbank made changes -
            Resolution Done [ 10000 ]
            Status To Do [ 10001 ] Done [ 10002 ]
            swinbank John Swinbank made changes -
            Link This issue relates to DM-2429 [ DM-2429 ]
            swinbank John Swinbank made changes -
            Component/s meas_extensions_photometryKron [ 12318 ]
            Component/s meas_extensions_kron [ 10737 ]

              People

              Assignee:
              pgee Perry Gee
              Reporter:
              pgee Perry Gee
              Reviewers:
              John Swinbank
              Watchers:
              Jim Bosch, John Swinbank, Perry Gee, Robyn Allsman [X] (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: