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

Ensure masks are valid from ImageMapReduceTask

    Details

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

      Description

      Ensure that the reducer subtask correctly sets the mask to invalid for pixels where the reduced exposure is infinite or NaN.

        Attachments

          Issue Links

            Activity

            Hide
            reiss David Reiss added a comment -

            For now, I am also creating a new mask plane, INVALID_IMAGEMAPREDUCE.
            This is meant to be used for debugging, and will be removed in the future (after testing and debugging on actual data is completed as part of epic DM-9441).

            Show
            reiss David Reiss added a comment - For now, I am also creating a new mask plane, INVALID_IMAGEMAPREDUCE. This is meant to be used for debugging, and will be removed in the future (after testing and debugging on actual data is completed as part of epic DM-9441 ).
            Hide
            reiss David Reiss added a comment -

            Nate Lust Would you be willing to review this ticket? It is a minor tweak to a set of classes which I previously wrote for performing operations on a grid across an image – to do spatially-varying operations. This ticket is just to ensure that the mask of the resulting image is being set correctly in the case of a failure or invalid pixels. Thanks a lot!

            Show
            reiss David Reiss added a comment - Nate Lust Would you be willing to review this ticket? It is a minor tweak to a set of classes which I previously wrote for performing operations on a grid across an image – to do spatially-varying operations. This ticket is just to ensure that the mask of the resulting image is being set correctly in the case of a failure or invalid pixels. Thanks a lot!
            Hide
            nlust Nate Lust added a comment -

            I don't see a good place to put this on Github, so I am going to leave a few comments on hear as well as Github.

            • I see that in the commit message for commit: 765cad0, the title is too long, pleas find a shorter descriptive title, and make the body of the commit message longer if need be.
            • The commit message contains the word also, which is not always a bad thing, but in this case I think the the work for these two feature would be better split up over two commits.
            Show
            nlust Nate Lust added a comment - I don't see a good place to put this on Github, so I am going to leave a few comments on hear as well as Github. I see that in the commit message for commit: 765cad0, the title is too long, pleas find a shorter descriptive title, and make the body of the commit message longer if need be. The commit message contains the word also, which is not always a bad thing, but in this case I think the the work for these two feature would be better split up over two commits.
            Hide
            nlust Nate Lust added a comment -

            Another general question is, why are you adding and populating the mask plane in this way? Why not just add the mask plane before the algorithm runs, populate it when failures occur, and if no failures occur at run-time remove the unneeded mask plane. The way you are doing things requires looping over all the pixels in an image many multiple times.

            I will continue to add comments to the code you did write, but I really think changing this to happen at run-time would be a better solution.

            Show
            nlust Nate Lust added a comment - Another general question is, why are you adding and populating the mask plane in this way? Why not just add the mask plane before the algorithm runs, populate it when failures occur, and if no failures occur at run-time remove the unneeded mask plane. The way you are doing things requires looping over all the pixels in an image many multiple times. I will continue to add comments to the code you did write, but I really think changing this to happen at run-time would be a better solution.
            Hide
            nlust Nate Lust added a comment -

            I have put some comments up on Github. Overall I am curious if this is a sort term research project code-base, or if it is intended to be a long term part of the stack kind of code-base. If the former there is a lot more wiggle room in my review (willing to go with certain things for the sake of time), if it is the latter I feel a longer discussion may be useful.

            Show
            nlust Nate Lust added a comment - I have put some comments up on Github. Overall I am curious if this is a sort term research project code-base, or if it is intended to be a long term part of the stack kind of code-base. If the former there is a lot more wiggle room in my review (willing to go with certain things for the sake of time), if it is the latter I feel a longer discussion may be useful.
            Hide
            nlust Nate Lust added a comment -

            As per our discussion, I am satisfied you have thought through the issues raised enough for this work to move forward.

            Show
            nlust Nate Lust added a comment - As per our discussion, I am satisfied you have thought through the issues raised enough for this work to move forward.
            Hide
            reiss David Reiss added a comment -

            Thanks, Nate.
            I created a new ticket: DM-10954 to capture additional work raised by our discussion.

            Show
            reiss David Reiss added a comment - Thanks, Nate. I created a new ticket: DM-10954 to capture additional work raised by our discussion.
            Hide
            reiss David Reiss added a comment -
            Show
            reiss David Reiss added a comment - Jenkins build passes: https://ci.lsst.codes/job/stack-os-matrix/24579/ Merged.

              People

              • Assignee:
                reiss David Reiss
                Reporter:
                reiss David Reiss
                Reviewers:
                Nate Lust
                Watchers:
                David Reiss, Nate Lust
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel