# Cleanup pybind11 code in meas_deblender

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• 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.

#### Activity

Hide
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
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
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
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
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
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
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
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
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
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
Fred Moolekamp added a comment -

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

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

Review complete. Just lots of little things.

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

#### People

Assignee:
Fred Moolekamp
Reporter:
Fred Moolekamp
Reviewers:
Jim Bosch
Watchers:
Fred Moolekamp, Jim Bosch