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-9887: Wrap meas_mosaic with pybind11 instead of swig #6

Merged
merged 1 commit into from Mar 23, 2017

Conversation

pschella
Copy link

No description provided.

Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

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

Review complete. I've pointed out a few things that probably should not be wrapped (some are just unnecessary; others may be dangerous in Python), and everything else looks fine.

# see <https://www.lsstcorp.org/LegalNotices/>.
#/

"""lsst...meas.mosaic
Copy link
Member

Choose a reason for hiding this comment

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

Extra dots? Unnecessary docstring.

cls.def_readwrite("coeffs", &Chev::coeffs);

return mod.ptr();
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this header needs to be wrapped at all - I don't see it in the %includes in mosaicLib.i.

cls.def_readwrite("xorder", &FluxFitParams::xorder, py::return_value_policy::copy);
cls.def_readwrite("yorder", &FluxFitParams::yorder, py::return_value_policy::copy);
cls.def_readwrite("absolute", &FluxFitParams::absolute);
cls.def_readwrite("coeff", &FluxFitParams::coeff, py::return_value_policy::copy);
Copy link
Member

Choose a reason for hiding this comment

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

xorder, yorder, and coeff seem to be used as arrays, so I'm not sure this wrapping will work for those. I'm fairly confident those could never be used from Python and it's probably best just to not wrap them at all.

cls.def_readwrite("order", &Class::order);
cls.def_readwrite("ncoeff", &Class::ncoeff);
cls.def_readwrite("xorder", &Class::xorder, py::return_value_policy::copy);
cls.def_readwrite("yorder", &Class::yorder, py::return_value_policy::copy);
Copy link
Member

Choose a reason for hiding this comment

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

As with the xorder and yorder attributes on FluxFitParams, these should not be wrapped, as they'll only cause trouble in Python.

cls.def_readwrite("a", &Class::a, py::return_value_policy::copy);
cls.def_readwrite("b", &Class::b, py::return_value_policy::copy);
cls.def_readwrite("ap", &Class::ap, py::return_value_policy::copy);
cls.def_readwrite("bp", &Class::bp, py::return_value_policy::copy);
Copy link
Member

Choose a reason for hiding this comment

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

These pointers are used as arrays and should not be accessible in Python.

@pschella pschella merged commit 4838653 into master Mar 23, 2017
@ktlim ktlim deleted the tickets/DM-9887 branch August 25, 2018 06:50
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