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
tickets/DM-16384 #72
tickets/DM-16384 #72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a couple of minor style comments.
python/lsst/meas/modelfit/mixture.cc
Outdated
(void (Mixture::*)(ndarray::Array<Scalar const, 1, 1> const &, | ||
ndarray::Array<Scalar,1,1> const &, | ||
ndarray::Array<Scalar,2,1> const &) const) | ||
&Mixture::evaluateDerivatives, "x"_a, "gradient"_a, "hessian"_a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use py::overload_cast
instead.
hessian.deep() = 0.0; | ||
gradient.setZero(); | ||
|
||
if (computeHessian) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add an assert(hessian)
here if internal logic is supposed to guarantee that it's always nonnull when computeHessian
is true, or an exception-throwing check if there's no internal guarantee on that.
include/lsst/meas/modelfit/Mixture.h
Outdated
void evaluateDerivativesImpl(A const & x, | ||
B & gradient, | ||
C * hessian, | ||
bool computeHessian = true) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to start this with an underscores (we aren't universally consistent about whether to do that on all private methods, but we do seem to be doing it in other methods in this class).
Add Eigen interfaces to some Mixure methods in addition to ndarray interfaces
9929c80
to
0ec9d1f
Compare
Update Mixure api to use Eigen
Add Eigen interfaces to some Mixure methods in addition to ndarray
interfaces