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-14690: Add ability to construct centered boxes #361

Merged
merged 2 commits into from Jun 26, 2018

Conversation

kfindeisen
Copy link
Member

This PR modifies the implementation of Exposure::getCutout to use the Box2I::makeCenteredBox factory introduced by lsst/geom#4. However, as a side effect the getCutout method became considerably more complex (17 LLOC for input validation plus three logical processing steps); any suggestions on how to break it up would be appreciated.

buffer << "Cannot create bounding box with dimensions " << size;
throw LSST_EXCEPT(pex::exceptions::InvalidParameterError, buffer.str());
}
lsst::geom::Box2I bbox = lsst::geom::Box2I::makeCenteredBox(pixelCenter, size, false);
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a good way to simplify this, though I'm also not too bothered by that - the overall code complexity is the same; you've just moved this check from one place to another.

But it may be worth noting that given the check for both the center coordinate being in within the Exposure bbox and the size being positive, I believe it's guaranteed that overlapBox.isEmpty() is always false up in _copyCommonPixels, and could perhaps be turned into an assert, if this is the only caller.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to leave it in, in case somebody later wants to promote _copyCommonPixels to a public method or function. I had quite a bit of trouble generalizing makeCenteredBox to work correctly with negative boxes, and assign's behavior is pretty surprising.

Copy link
Member

Choose a reason for hiding this comment

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

assign's behavior is pretty surprising

I had no idea that's how it worked until I saw your comment, and now that I do know, it makes me sad, too.

@RobertLuptonTheGood
Copy link
Member

RobertLuptonTheGood commented Jun 16, 2018 via email

@kfindeisen
Copy link
Member Author

As stated in the getCutout method documentation, pixels off the edge of the image will be zero. This behavior is consistent with the behavior of the "blank" Exposure constructors. Note that this PR does not change the behavior of getCutout in any way.

NaNs are not a terribly practical choice for the filler because we would need to adopt a different convention anyway for integer Exposures (of which we have three mandatory instantiations ATM). It could be done, of course, but it would make the behavior more surprising to users.

Setting mask bits is not possible because Exposure/MaskedImage knows nothing about the meaning of specific bits -- those are determined by client code.

@TallJimbo
Copy link
Member

I should have thought of this this on the previous ticket, but I think the NO_DATA mask plane's meaning is sufficiently global and appropriate that it probably would make sense to set that for any off-the-exposure pixels.

@kfindeisen
Copy link
Member Author

After some Slack discussion that doesn't seem as unreasonable as I first thought. I've logged the feature request as DM-14814.

@kfindeisen kfindeisen merged commit 1ad5455 into master Jun 26, 2018
@kfindeisen kfindeisen deleted the tickets/DM-14690 branch August 22, 2018 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants