DM-14690: Add ability to construct centered boxes #4
Conversation
The previous documentation said that the point passed to the constructor was the lower left corner; this is only true if the requested dimensions are positive.
kfindeisen
force-pushed
the
tickets/DM-14690
branch
from
c527015
to
ee95663
Compare
include/lsst/geom/Box.h
Outdated
| * If the returned box is not empty, its center shall be within | ||
| * half a pixel of `center` in either dimension. | ||
| */ | ||
| static Box2I makeCenteredBox(Point2D const& center, Extent const& size, bool invert = true); |
There was a problem hiding this comment.
There's actually an adopted RFC for changing the default for invert in the constructors to false. That work hasn't been done primarily because it's not backwards-compatible, and hence requires some care and effort to track down breakage. Since this is a new API it might be better to use invert=false from the start or even drop that argument from this signature entirely, unless you have a use case for it.
There was a problem hiding this comment.
I think if we're going to mix factory methods and constructors in APIs, we should require that they behave consistently. Anything else will lead to user error.
I could set invert=false, but by the same token I'd be worried if the RFC didn't get implemented quickly and we were stuck with inconsistent behavior for an extended period of time.
There was a problem hiding this comment.
What about just removing it? While I am concerned about blithely changing the default without taking care to avoid breakage, I think it's also true that invert is essentially never used, and if we don't have it here there'd be no expectation of consistency.
There was a problem hiding this comment.
Actually, I consider removing it the worst option, barring an RFC to remove invert from the constructors as well.
The problem with defaults is that they tend to hide parameters from users. In this case, a developer who prefers to learn APIs from example rather than by consulting the documentation could well know that Box(myPoint, Extent(-3, -4)) returns a 3×4 box without knowing that Box(myPoint, Extent(-3, -4), false) is even a valid call. To such a developer, the absence of an invert parameter in makeCenteredBox would by no means imply that it interprets dimensions differently.
If we make invert=false by default, that wouldn't fix the problem of surprising default behavior, but at least the user would have an easy workaround once they realized what was going on.
There was a problem hiding this comment.
I do think not having invert is the right long-term signature for both the constructors and this function, and I have a slight preference for doing whatever makes it easiest to migrate to that state. But I won't object if you'd rather emphasize consistency in the current interface either a little (by retaining invert and making it default to False) or a lot (by retaining it with a default of true). The future and the present are both worthwhile things to emphasize.
include/lsst/geom/Box.h
Outdated
| * If the returned box is not empty, it shall be centered | ||
| * on `center`. | ||
| */ | ||
| static Box2D makeCenteredBox(Point2D const& center, Extent const& size, bool invert = true); |
There was a problem hiding this comment.
Same point as for Box2I on the default for invert.
kfindeisen
force-pushed
the
tickets/DM-14690
branch
from
ee95663
to
60bf70d
Compare
Box2I::makeCenteredBox and Box2D::makeCenteredBox make it possible to quickly construct boxes centered on a specific point within a pixel. For consistency with the constructor behavior introduced in RFC-324, the factory methods never invert the requested dimensions.
kfindeisen
force-pushed
the
tickets/DM-14690
branch
from
60bf70d
to
33d0814
Compare
This PR adds factory methods to
Box2*that create a box centered on a (fractional) point. In the process of debugging the implementation, I discovered that the behavior of one of theBoxconstructors is more complex than documented, and updated the documentation to match the existing behavior.The renaming of the
Box2*constructors' first parameter fromminimumtocornerdoes not seem to have broken any keyword-based code inlsst_distrib.