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

Cleanup pybind11 code in meas_deblender

    Details

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

      Description

      Use the checklist from DM-9182 to clean up code in meas_deblender.

        Attachments

          Issue Links

            Activity

            Hide
            fred3m Fred Moolekamp added a comment -

            The file structure is not completely inline with RFC-281 and the pybind11 style guide, as the header Baseline.h has the wrapper file baseline.cc that does not wrap any classes, however there also exists a file baseline.py that defines several classes and functions. While it might be advisable to rename baseline.py at some point (perhaps when the new deblender is merged), I'm not sure that breaking the API in this way is within the scope of the pybind11 port.

            Show
            fred3m Fred Moolekamp added a comment - The file structure is not completely inline with RFC-281 and the pybind11 style guide , as the header Baseline.h has the wrapper file baseline.cc that does not wrap any classes, however there also exists a file baseline.py that defines several classes and functions. While it might be advisable to rename baseline.py at some point (perhaps when the new deblender is merged), I'm not sure that breaking the API in this way is within the scope of the pybind11 port.
            Hide
            jbosch Jim Bosch added a comment -

            I'm quite confident there's no code outside `meas_deblender` that imports `baseline.py` or otherwise knows about its existence. I'd advocate just renaming one or perhaps both files, since "baseline" is a terrible name (which I know you didn't come up with).

            Show
            jbosch Jim Bosch added a comment - I'm quite confident there's no code outside `meas_deblender` that imports `baseline.py` or otherwise knows about its existence. I'd advocate just renaming one or perhaps both files, since "baseline" is a terrible name (which I know you didn't come up with).
            Hide
            fred3m Fred Moolekamp added a comment -

            Changing the names may be a bit more complicated than it appears, and not worth the trouble for code that is likely to be deprecated within the next month or two. ip_diffim imports baseline.py as well as a large number of scripts written by myself, Bob Armstrong, and possibly Robert Lupton, to gain access to internal deblender classes for testing. So I would really rather not change the name of the python file baseline.py for now. Not to mention that there will be another API change shortly when the new footprints are added to the pybind11 code, and this could complicate Nate Lust's work.

            Since the C++ code includes all of the functions in a single class BaselineUtilsF, we could rename the source code files to BaselineUtils.cc, BaselineUtils.h with the wrapper baselineUtils.cc. Again, if I thought that this code would last more than a month I would suggest renaming the files and the class to something more reasonable, but the new deblender doesn't require any of those function and they are all likely to be deprecated, so it just doesn't seem worth the trouble to spend too much time on renaming files and classes.

            Show
            fred3m Fred Moolekamp added a comment - Changing the names may be a bit more complicated than it appears, and not worth the trouble for code that is likely to be deprecated within the next month or two. ip_diffim imports baseline.py as well as a large number of scripts written by myself, Bob Armstrong , and possibly Robert Lupton , to gain access to internal deblender classes for testing. So I would really rather not change the name of the python file baseline.py for now. Not to mention that there will be another API change shortly when the new footprints are added to the pybind11 code, and this could complicate Nate Lust 's work. Since the C++ code includes all of the functions in a single class BaselineUtilsF , we could rename the source code files to BaselineUtils.cc , BaselineUtils.h with the wrapper baselineUtils.cc . Again, if I thought that this code would last more than a month I would suggest renaming the files and the class to something more reasonable, but the new deblender doesn't require any of those function and they are all likely to be deprecated, so it just doesn't seem worth the trouble to spend too much time on renaming files and classes.
            Hide
            jbosch Jim Bosch added a comment -

            Your BaselineUtils suggestion sounds like an excellent resolution to me. And I'm very glad you caught my bad assertion on whether that code was used anywhere else. I'm also a bit disappointed there is code that uses it in ip_diffim - I can't imagine how that could a be a good idea. As a side note, is there any chance we could get those testing scripts (or at least some utility code they use) moved into meas_deblender or some other stack package in the future? The more we can get in CI, the better.

            Show
            jbosch Jim Bosch added a comment - Your BaselineUtils suggestion sounds like an excellent resolution to me. And I'm very glad you caught my bad assertion on whether that code was used anywhere else. I'm also a bit disappointed there is code that uses it in ip_diffim - I can't imagine how that could a be a good idea. As a side note, is there any chance we could get those testing scripts (or at least some utility code they use) moved into meas_deblender or some other stack package in the future? The more we can get in CI, the better.
            Hide
            fred3m Fred Moolekamp added a comment -

            Absolutely. There are a number of new tests that will be included in meas_deblender once we implement the new NMF algorithm. Once the new deblender stabilizes and we are ready to merge to master, I plan to implement all of the tests and examples that I have created.

            Show
            fred3m Fred Moolekamp added a comment - Absolutely. There are a number of new tests that will be included in meas_deblender once we implement the new NMF algorithm. Once the new deblender stabilizes and we are ready to merge to master, I plan to implement all of the tests and examples that I have created.
            Hide
            fred3m Fred Moolekamp added a comment -

            meas_deblender has been cleaned up, per RFC. All tests pass in python 2 and 3 on Jenkins.

            Show
            fred3m Fred Moolekamp added a comment - meas_deblender has been cleaned up, per RFC. All tests pass in python 2 and 3 on Jenkins.
            Hide
            jbosch Jim Bosch added a comment -

            Review complete. Just lots of little things.

            Show
            jbosch Jim Bosch added a comment - Review complete. Just lots of little things.

              People

              • Assignee:
                fred3m Fred Moolekamp
                Reporter:
                fred3m Fred Moolekamp
                Reviewers:
                Jim Bosch
                Watchers:
                Fred Moolekamp, Jim Bosch
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel