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-15431: expand pybind11 interface #113

Merged
merged 1 commit into from Nov 7, 2018
Merged

DM-15431: expand pybind11 interface #113

merged 1 commit into from Nov 7, 2018

Conversation

parejkoj
Copy link
Collaborator

No description provided.

@PaulPrice
Copy link
Contributor

The commit messages violate the guidelines for commit messages:

  • the first is not in the imperative tense;
  • the second is vague and seems to refer to some other work ("more") that requires context to understand;
  • neither contain any information that informs the reader about what has been changed, and why it's necessary.

Copy link
Contributor

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

I don't see why this is included in this ticket since it doesn't seem related, but...

@@ -49,6 +49,11 @@ void declareGtransfo(py::module &mod) {
"inputframe"_a, "inscribed"_a);
cls.def("getNpar", &Gtransfo::getNpar);
cls.def("offsetParams", &Gtransfo::offsetParams);
cls.def("computeDerivative", [](Gtransfo const &self, Point const &where, const double step) {
Copy link
Contributor

Choose a reason for hiding this comment

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

double const is LSST standard.

cls.def("computeDerivative", [](Gtransfo const &self, Point const &where, const double step) {
GtransfoLin derivative;
self.computeDerivative(where, derivative, step);
return derivative;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this pattern (create a GtransfoLin, pass it in and then return it) indicates that the computeDerivative method should have an API that returns a new derivative object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The existing C++ uses of computeDerivative almost all reuse an existing array for the output. We could look at those and see if they are worth turning into what you suggest, but that's not for this ticket.

Although not used in python currently, these methods were useful for some
exploratory work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants