tickets/DM-8175 #54
tickets/DM-8175 #54
Conversation
| } | ||
|
|
||
| afw::geom::Box2I overallBBox; | ||
| for (afw::table::ExposureCatalog::const_iterator i = subcat.begin(); i != subcat.end(); ++i) { |
There was a problem hiding this comment.
I'm not sure if this would work or not, but I'm curious if this line might be clearer as
for (auto&& i : subcat) {
of even
for (auto&& cat : subcat) {
There was a problem hiding this comment.
I think you'd just want for (auto i : subcat) {.
There was a problem hiding this comment.
without the &&, it tries to copy the elements into i:
src/CoaddPsf.cc:269:15: error: call to implicitly-deleted copy constructor of 'lsst::afw::table::ExposureRecord'
for (auto exposureRecord : subcat) {
I'll change all the loops over the catalog to c++11 style, in a separate commit.
There was a problem hiding this comment.
Oh, I'm an idiot. Of course; I was thinking that i would be an iterator type, not the record type. However, I think the appropriate invocation is then auto const & record; we want the element type to resolve to an lvalue const reference ExposureRecord const &, not an rvalue reference ExposureRecord &&.
There was a problem hiding this comment.
This is very interesting. I was trying to guess the best choice range declaration from the examples here. auto const & certainly sounds like a better option though, now that I know it exists!
There was a problem hiding this comment.
Yeah, type deduction for auto is pretty weird. I remember thinking at one point that it all made sense, now that I understood it, but even though I now remember at least most of the rules I no longer remember why it had to be that way.
| @@ -292,5 +296,9 @@ BOOST_AUTO_TEST_CASE(warpedPsf) | |||
| // TODO: improve this test; the ideal thing would be to repeat with | |||
| // finer resolutions and more stringent threshold | |||
| BOOST_CHECK(compare(*im,*im2) < 0.005); | |||
|
|
|||
| //Check that computeBBox returns same dimensions as image | |||
| BOOST_CHECK((warped_psf->computeBBox().getWidth() - nx) == 0); | |||
There was a problem hiding this comment.
Why not BOOST_CHECK(warped_psf->computeBBox().getWidth() == nx); ?
| afw::geom::AffineXYTransform xyTransform(t); | ||
|
|
||
| afw::math::SeparableKernel const& kernel = *wc.getWarpingKernel(); | ||
| afw::geom::Point2I const& center = kernel.getCtr(); | ||
| int const xPad = std::max(center.getX(), kernel.getWidth() - center.getX()); | ||
| int const yPad = std::min(center.getY(), kernel.getHeight() - center.getY()); | ||
| int const yPad = std::max(center.getY(), kernel.getHeight() - center.getY()); |
There was a problem hiding this comment.
Does this change suggest we're missing a unit test that would have caught this?
There was a problem hiding this comment.
I've written a unit test for this and will move the discussion to the Jira ticket, because I want to attach pictures: https://jira.lsstcorp.org/browse/DM-8175
| (afw::geom::Box2I const bbox, | ||
| afw::geom::AffineTransform const &t | ||
| ) { | ||
| static const double maxTransformCoeff = 200.0; |
There was a problem hiding this comment.
Since you're checking against maxTransformCoeff here, I think you can probably delete the check in warpAffine. Should probably bring the comment block down from there to here, though.
yalsayyad
force-pushed
the
tickets/DM-8175
branch
4 times, most recently
from
6b195c7
to
f7bff77
Compare
Original tolerance (0.005) was too stringent for 2.5% of random Psfs and transforms.
yalsayyad
force-pushed
the
tickets/DM-8175
branch
from
f7bff77
to
181188a
Compare
This ticket contains computeBBox implementations for WarpedPsf, CoaddPsf, and KernelPsf (trivial).
This ticket also includes commit to reduce some of the code duplication in testCoaddPsf, and a change to make consistent the x/y padding to the PSF image before warping it. The original author of that line (81 in WarpedPsf.cc) agrees with the change.