Fix centering bug reported in #1322 #1323
Conversation
rmjarvis
added
base classes
| b = b.shift(_PositionI(np.floor(center.x+0.5)-b.center.x, | ||
| np.floor(center.y+0.5)-b.center.y)) | ||
| b = b.shift(_PositionI(np.floor(center.x+0.5-b.true_center.x), | ||
| np.floor(center.y+0.5-b.true_center.y))) |
There was a problem hiding this comment.
Worth mentioning that here is the other line, which this should be consistent with (and wasn't).
There was a problem hiding this comment.
Thanks for the fix!
I have two related questions, none of them should block this PR but might be good fodder for future work.
-
Is it worth trying to systematically add tests that exercise all of the drawing options in each combination? I was surprised to find this bug and it reminded me that even if the coverage is near 100%, we can still miss bugs because we have not hit all possible paths through the code.
-
Is it worth trying to do some internal refactoring on some of the private methods to avoid the need to have certain lines of code be kept in sync? For example, I wonder if it is possible to refactor things such that the wcs, bounds, etc is all made up front and then the image is generated from those. This might help different bits of the code be more robust to changes maybe?
In any case, LGTM and thank you!
|
|
Sounds good. Thank you and LGTM! |
cf. #1322
@beckermr @zhouconghao