Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-9190: Cleanup pybind11 remaining code #51

Merged
merged 1 commit into from Apr 6, 2017
Merged

Conversation

pschella
Copy link

No description provided.

Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think imports are needed in these wrappers. Most of them need lsst.afw.math (for Kernel and such). Many use other afw sub-packages, some use lsstpex.policy. In addition I suspect some of the wrappers use contents of other wrappers.

@pschella pschella force-pushed the tickets/DM-9190 branch 2 times, most recently from ecf2c15 to 687cee9 Compare March 26, 2017 14:04
Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a few missing imports

PYBIND11_PLUGIN(_kernelCandidateDetection) {
py::module mod("_kernelCandidateDetection", "Python wrapper for KernelCandidateDetection.h");
PYBIND11_PLUGIN(kernelCandidateDetection) {
py::module::import("lsst.pex.policy");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this code also needs lsst.afw.image and lsst.afw.detection

py::module mod("_kernelSolution", "Python wrapper for KernelSolution.h");
PYBIND11_PLUGIN(kernelSolution) {
py::module::import("lsst.afw.math");
py::module::import("lsst.pex.policy");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need lsst.afw.geom and lsst.afw.image

@@ -136,7 +138,10 @@ void declarePsfDipoleFlux(py::module &mod) {
} // namespace lsst::ip::diffim::<anonymous>

PYBIND11_PLUGIN(_dipoleAlgorithms) {
py::module mod("_dipoleAlgorithms", "Python wrapper for DipoleAlgorithms.h");
py::module::import("lsst.afw.table");
py::module::import("lsst.meas.base");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is lsst.pex.config needed for the control fields?

from .kernelCandidate import *
from .kernelCandidateDetection import *
from .kernelSolution import *

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only package of the set, so far, that has retained its xLib.py file. Is this intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it was. This is difficult to remove and expected to be restored in DM-9786.
Thus leave it in for now until a decision has been made on that ticket.

PYBIND11_PLUGIN(_kernelSumVisitor) {
py::module mod("_kernelSumVisitor", "Python wrapper for KernelSumVisitor.h");
PYBIND11_PLUGIN(kernelSumVisitor) {
py::module::import("lsst.afw.math");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this also needs lsst.pex.policy (used by makeKernelSumVisitor).

PYBIND11_PLUGIN(_basisLists) {
py::module mod("_basisLists", "Python wrapper for BasisLists.h");
PYBIND11_PLUGIN(basisLists) {
py::module mod("basisLists");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this needs to import lsst.afw.math and lsst.pex.policy

Unlike other cleanup I left in the diffimLib module.
This is difficult to remove and expected to be restored in DM-9786.
Thus leave it in for now until a decision has been made on that ticket.
@pschella pschella merged commit cc6134a into master Apr 6, 2017
@ktlim ktlim deleted the tickets/DM-9190 branch August 25, 2018 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants