Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-680

Add Kernel Hough Transform as third-party package

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      Add KHT (Kernel Hough Transform) to lsst_distrib as a third-party package (https://github.com/laffernandes/kht) This package is needed by the satellite-trail finding task that will be incorporated into building coadds. The kernel-based hough transform provides a significant speed-up relative to a traditional hough transform.

      For the stack, the structure of the existing package will be reworked to use the python wrapper pybind11 and to fit into the DM stack package template.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            Is the pybind11 interface identical to the boost::python interface such that you could use the original KHT without changing any LSST code?

            Show
            tjenness Tim Jenness added a comment - Is the pybind11 interface identical to the boost::python interface such that you could use the original KHT without changing any LSST code?
            Hide
            csaunder Clare Saunders added a comment -

            As it stands, the python module has the same name in our pybind11 implementation as the boost implementation. Currently we have used different names to wrap the main function. These are both arbitrary choices. I am adding some functionality to the code that will preclude the interface from being exactly the same.

            I would say that the user would just install the wrapper that they want, so there shouldn't be confusion having two implementations with the same name.

            Show
            csaunder Clare Saunders added a comment - As it stands, the python module has the same name in our pybind11 implementation as the boost implementation. Currently we have used different names to wrap the main function. These are both arbitrary choices. I am adding some functionality to the code that will preclude the interface from being exactly the same. I would say that the user would just install the wrapper that they want, so there shouldn't be confusion having two implementations with the same name.
            Hide
            tjenness Tim Jenness added a comment -

            If we are forking this package and changing its interface then we have to give it a different name and acknowledge where it came from. Many software licenses won't let us use the same name if we fork like this.

            Show
            tjenness Tim Jenness added a comment - If we are forking this package and changing its interface then we have to give it a different name and acknowledge where it came from. Many software licenses won't let us use the same name if we fork like this.
            Hide
            csaunder Clare Saunders added a comment -

            After discussion between Tim Jenness, John Swinbank, Nate Lust, and myself, we decided that the right approach is to have a package with the name 'kht', which be imported in python with 'from lsst import kht'. We will add a README that emphasizes the fact that the lsst kht is different from the upstream kht and that they are not interchangeable.

            Show
            csaunder Clare Saunders added a comment - After discussion between Tim Jenness, John Swinbank, Nate Lust, and myself, we decided that the right approach is to have a package with the name 'kht', which be imported in python with 'from lsst import kht'. We will add a README that emphasizes the fact that the lsst kht is different from the upstream kht and that they are not interchangeable.
            Hide
            tjenness Tim Jenness added a comment -

            I agree with the proposed outcome. Please adopt this RFC.

            Show
            tjenness Tim Jenness added a comment - I agree with the proposed outcome. Please adopt this RFC.

              People

              Assignee:
              csaunder Clare Saunders
              Reporter:
              csaunder Clare Saunders
              Watchers:
              Clare Saunders, Jim Bosch, John Swinbank, Kian-Tat Lim, Tim Jenness, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End: