-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix boundingbox transformation #591
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but need to consider how a poinset creates its enclosing bounding box.
return self.__class__( | ||
point1=self.minpoint.warp(spaceobj), | ||
point2=self.maxpoint.warp(spaceobj), | ||
space=spaceobj, | ||
) | ||
return self.corners.warp(spaceobj).boundingbox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since boundingbox of a pointset shifts the min and max points 1e-6m, this change causes the boundingbox to expand 1 voxel in each direction. (see line 245-257 in pointset.py).
I suggest either:
- actually finding the min and max corner points to create bounding box or,
- Removing the offset the bounding box of a pointset has
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any idea why the shift was introduced in PointSet? I would say it should be fixed there if we do not have obvious reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it was there before I arrived and we never discussed it so I do not know. If I were to guess, to make sure the point is enclosed by the bounding box. But I think this shouldn't be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AhmetNSimsek The offset is for avoiding the case of a zero-volume bounding boxes with strict 2D shape, which would have no intersection with any other objects and could therefore not be used for filtering points or finding intersecting images. I think we can remove the numerical offset, but raise a warning whenever a zero volume bounding box is constructed, because this should hardly happen. For example, bounding boxes of 2D images must have nonzero volume if the 2D image lives in 3D space, and the boundingbox function of images needs to cover that. The depth is the pixel size then. I assume that zero volume bounding boxes mostly occurred from manual construction of annotations in some tutorials, and this minimum size has been introduced to overcome the resulting problems, but it is not a clean solution. My suggestion is to remove the offset, raise an exception when construction a zero-volume bounding box, then run all tests and tutorial notebooks to see when that happens. Ultimatlely the exception can become a warning.
…due to masking (data) issue" This reverts commit 99e140b.
The image feature e2e tests are failing because some warped corners of a bounding box can return None which causes the warping to create a bbox with (nan, nan, nan) points and such a bbox has no intersection with any well-defined bbox. I'll see how I can circumvent the issue. |
Wikibrain stem image feature cannot be found anymore when mni152 template is given since the warping fails
siibra had a serious bug when transforming bounding boxes: Both
.warp
and.transform
would create the new bounding box by just transforming the min- and max point. But since the bounding box is axis aligned, this is not correct - other corner points could form the new min- or maxpoint in the new coordinate system after warping. Therefore, all corner points (8 in general) need to be transformed, and a new min- and maxpoint have to be determined in the target space to create the transformed bounding box.