DM-38616: Add GSL interpolation for Sersic gaussian mixture #10
Conversation
| &g2f::GSLSersicMixInterpolator::correct_final_integral) | ||
| .def_property_readonly("final_correction", | ||
| &g2f::GSLSersicMixInterpolator::get_final_correction) | ||
| .def("integralsizes", &g2f::GSLSersicMixInterpolator::get_integralsizes) |
There was a problem hiding this comment.
For this and other methods with parameters, I recommend including them here to make the interface more user friendly, like you did for your init method.
src/gslsersicmixinterpolator.cc
Outdated
| if (!((sersicindex >= sersicindex_min) && (sersicindex <= sersicindex_max))) { | ||
| throw std::invalid_argument("sersicindex=" + std::to_string(sersicindex) | ||
| + " !(>=" + std::to_string(sersicindex_min) | ||
| + "&& <=" + std::to_string(sersicindex_max)); | ||
| } |
There was a problem hiding this comment.
Maybe rephrase this? Something like "sersicIndex is not between sersicindex_min and sersic index_max."
src/gslsersicmixinterpolator.cc
Outdated
| if (!(integral >= 0)) { | ||
| throw std::logic_error(this->str() + ".get_integralsizes(" + to_string_float(sersicindex) | ||
| + ") got correction=" + to_string_float(_final_correction) | ||
| + " from integral=" + to_string_float(integral) + " !>=0"); |
There was a problem hiding this comment.
The same goes for simplifying the rest of the error messages in this file.
| // It's not so obvious how a polynomial interpolator should behave | ||
| // (best not to use them with so few knots in the first place) | ||
| if (type != g2f::InterpType::polynomial) { | ||
| CHECK(interp_0 > (y[0] + y[1]) / 2.); | ||
| } | ||
| } |
There was a problem hiding this comment.
You could implement a test in python that compares your results to scipy spline results for other splines within a given tolerance. If you don't already have scipy as a dependency then you could just hard code the scipy results just to make sure that you don't accidentally do something wonky (not that I know anything about that).
There was a problem hiding this comment.
It would not make sense to add scipy as a dependency here, so I added functionality in multiprofit to plot scipy-interpolated values instead. As expected, they're nearly identical for the cubic spline.
You're right that I should add a test against absolute values. I think I will defer doing that until I'm convinced that the default knot values and spline method are optimal. In a sense, changing the interpolator will make it a different profile and change fitted parameter values so I will also discourage doing so in the future.
No description provided.