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

Implement NMF with alternating least squares

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      4
    • Sprint:
      DRP S17-3, DRP S17-4
    • Team:
      Data Release Production

      Description

      Peter Melchior will supply a pseudocode algorithm for this (which either he or Fred Moolekamp will make available on this ticket).

      Code will take in the peaks, constructs symmetry matrices, construct monotonicty matrices, apply constraints as appropriate, make these available to the proximal operators. This ticket includes defining the interfaces by which that information will be made available to the proximal operators, but not implementing those operators themselves, which are created in DM-9143 (monotonicity), DM-8959 (symmetry) and DM-9170 (PSF convolution).

        Attachments

          Issue Links

            Activity

            Hide
            fred3m Fred Moolekamp added a comment -

            I created a new module proximal.py for functions related to the NMF deblender that uses proximal operators and ADMM (see DM-9173). Most of these functions are nearly identical to those in nmf.py but with an improved API more conducive to interoperability with Peter Melchior's code.

            The basic content of the code is an ExposureDeblend object that is used to load and store the images in each band, a merged catalog, and (optionally) a "true" catalog if the image was created by a simulation (used for testing the deblender). For each parent a DeblendedParent object is created, which initializes the NMF matrices, creates the necessary operators (symmetry, monotonicity, and psf), and estimates the SED and Intensity matrices by calling the proximal operators being written by Peter Melchior in a loop until reaching {maxiter}}.

            Once the proximal operators are completed, this code will be tested using meas_deblender/examples/testNMFproximal.ipynb and put in review.

            Show
            fred3m Fred Moolekamp added a comment - I created a new module proximal.py for functions related to the NMF deblender that uses proximal operators and ADMM (see DM-9173 ). Most of these functions are nearly identical to those in nmf.py but with an improved API more conducive to interoperability with Peter Melchior 's code. The basic content of the code is an ExposureDeblend object that is used to load and store the images in each band, a merged catalog, and (optionally) a "true" catalog if the image was created by a simulation (used for testing the deblender). For each parent a DeblendedParent object is created, which initializes the NMF matrices, creates the necessary operators (symmetry, monotonicity, and psf), and estimates the SED and Intensity matrices by calling the proximal operators being written by Peter Melchior in a loop until reaching {maxiter}}. Once the proximal operators are completed, this code will be tested using meas_deblender/examples/testNMFproximal.ipynb and put in review.
            Hide
            fred3m Fred Moolekamp added a comment -

            I finally finished separating the branches and adding comments to make the code more understandable.

            You'll notice that a lot of the functions in proximal.py are very similar (or identical) to those in the old nmf.py file. This is because I expect to reorganize the module names long before we merge to master and I didn't want to break any of the NMF scripts that were created before we started using proximal operators until we're sure that this version works better (which it almost certainly will) and runs in a reasonable amount of time (which is yet to be determined, although we have seen some speedup using the most recent operators). However, interfacing with Peter Melchior's algorithms required a slightly different API and many of the display functions and internal classes had to be very slightly modified.

            Don't worry about proximal_nmf.py, that's Peter's code that is occasionally updated by me from his repo.

            Show
            fred3m Fred Moolekamp added a comment - I finally finished separating the branches and adding comments to make the code more understandable. You'll notice that a lot of the functions in proximal.py are very similar (or identical) to those in the old nmf.py file. This is because I expect to reorganize the module names long before we merge to master and I didn't want to break any of the NMF scripts that were created before we started using proximal operators until we're sure that this version works better (which it almost certainly will) and runs in a reasonable amount of time (which is yet to be determined, although we have seen some speedup using the most recent operators). However, interfacing with Peter Melchior 's algorithms required a slightly different API and many of the display functions and internal classes had to be very slightly modified. Don't worry about proximal_nmf.py, that's Peter's code that is occasionally updated by me from his repo.
            Hide
            fred3m Fred Moolekamp added a comment -

            Bob Armstrong will you be able to take a look at this early next week? If not I might need to find another reviewer because DM-9172 is built on top of it and is almost ready to be merged (to my user branch).

            Show
            fred3m Fred Moolekamp added a comment - Bob Armstrong will you be able to take a look at this early next week? If not I might need to find another reviewer because DM-9172 is built on top of it and is almost ready to be merged (to my user branch).
            Hide
            fred3m Fred Moolekamp added a comment -

            I switched the review to Peter Melchior, since he's basically reviewed this ticket already by approving DM-9172 and DM-9170. Since I can't merge those branches until after this ticket is merged, he agreed to review this ticket. Bob Armstrong, once the ticket is merged it would be best for you to look at my user branch on meas_deblender to see the latest version of the code, which has been improved since this ticket.

            Show
            fred3m Fred Moolekamp added a comment - I switched the review to Peter Melchior , since he's basically reviewed this ticket already by approving DM-9172 and DM-9170 . Since I can't merge those branches until after this ticket is merged, he agreed to review this ticket. Bob Armstrong , once the ticket is merged it would be best for you to look at my user branch on meas_deblender to see the latest version of the code, which has been improved since this ticket.
            Hide
            pmelchior Peter Melchior added a comment -

            Please pull from my latest commit. There are changes to the proximal operators to enable an additional configurable parameters which will act as the threshold for l1 or l0 sparsity constraints. Also implemented now is the PSF convolution in operator form (as enabled by DM-9170). Once these new features are in, we can close this ticket.

            Show
            pmelchior Peter Melchior added a comment - Please pull from my latest commit. There are changes to the proximal operators to enable an additional configurable parameters which will act as the threshold for l1 or l0 sparsity constraints. Also implemented now is the PSF convolution in operator form (as enabled by DM-9170 ). Once these new features are in, we can close this ticket.
            Hide
            rearmstr Bob Armstrong added a comment -

            Sorry, I did look at the code last week, but I was unsure of how exactly to review it. The overall structure looked fine, but my impression was that this wasn't being merged to master. So while I had a number of smaller comments, I thought it probably made more sense for those to wait until the merge to master.

            Show
            rearmstr Bob Armstrong added a comment - Sorry, I did look at the code last week, but I was unsure of how exactly to review it. The overall structure looked fine, but my impression was that this wasn't being merged to master. So while I had a number of smaller comments, I thought it probably made more sense for those to wait until the merge to master.
            Hide
            fred3m Fred Moolekamp added a comment -

            I agree Bob Armstrong, this is not being merged to master. I just wanted to get the ticket reviewed so I can merge it to my user branch, since I have two other ticket branches on top of this one waiting to be merged so I can close out my work for the sprint. Peter Melchior and I will still be iterating on the changes for at least another sprint, and then likely another sprint or two of optimizing the code and creating tests before it is ready to be merged to master.

            Peter Melchior, using the same line of reasoning I think it will be easier to merge your changes with DM-9170, since there have been a number of changes to the code in DM-9170 and DM-9172 since this ticket was first put in review and including them in this ticket would take more time than it is worth. Plus, since DM-9170 is the ticket for creating the PSF convolution operator I think that would be the more appropriate branch to use for this change.

            Show
            fred3m Fred Moolekamp added a comment - I agree Bob Armstrong , this is not being merged to master. I just wanted to get the ticket reviewed so I can merge it to my user branch, since I have two other ticket branches on top of this one waiting to be merged so I can close out my work for the sprint. Peter Melchior and I will still be iterating on the changes for at least another sprint, and then likely another sprint or two of optimizing the code and creating tests before it is ready to be merged to master. Peter Melchior , using the same line of reasoning I think it will be easier to merge your changes with DM-9170 , since there have been a number of changes to the code in DM-9170 and DM-9172 since this ticket was first put in review and including them in this ticket would take more time than it is worth. Plus, since DM-9170 is the ticket for creating the PSF convolution operator I think that would be the more appropriate branch to use for this change.

              People

              • Assignee:
                fred3m Fred Moolekamp
                Reporter:
                swinbank John Swinbank
                Reviewers:
                Peter Melchior
                Watchers:
                Bob Armstrong, Fred Moolekamp, John Swinbank, Peter Melchior
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel