Review for DM-5197: new shapelet PSF approximation code #21
Conversation
| @@ -238,8 +238,7 @@ struct CModelControl { | |||
| LSST_CONTROL_FIELD( | |||
| psfName, | |||
There was a problem hiding this comment.
Hmm. Maybe. Field names like these are really sequences of namespace-like terms, so I'm not sure whether calling them a "prefix" is warranted (it feels like calling the directory part of a full filename a prefix). Regardless, it's clear that the documentation string below needs to be improved.
|
About the range checking. Doesn't |
|
And concerning |
| afw::image::Image<Scalar> const & image | ||
| ) : OptimizerObjective(image.getBBox().getArea(), 4), | ||
| _minRadius(ctrl.minRadius), | ||
| _maxRadius(ctrl.maxRadiusBoxFraction * std::sqrt(this->dataSize)), |
There was a problem hiding this comment.
No, this is just intended to be a geometric mean of the width and height. But I do think the box will be close enough to square that I'm not concerned about the difference between geometric and arithmetic mean (there really isn't a good reason to pick one or the other).
This will eventually allow us to use analytic derivatives instead of finite differences.
The current algorithm is not very robust and is sometimes catastrophically slow. However, it's able to fit much more general models than the HSC-side one we'll be adding shortly, so we'd like to keep it around.
This task is slated for eventual removal, but that's more work than we want to do on this ticket. For now, we just need to remove a name collision.
Yes, but what matters is that the array shapes agree with the shape of the |
My preference would probably be for adding an |
TallJimbo
force-pushed
the
tickets/DM-5197
branch
from
0daccd0
to
c5d4e6d
Compare
Many of these passed on my Ubuntu box, but not on CentOS CI.
No description provided.