-
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
Add Volume.evaluate_points() and fix voxel access for _points_inside #566
Conversation
…uate_points
siibra/volumes/volume.py
Outdated
""" | ||
Reduce a pointset to the points which fall | ||
inside nonzero pixels of this volume. | ||
The indices of the original points are stored as point labels in the result. |
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.
in fact the given points might have labels already, which the user doesn't want to be overwritten by the indices in the input pointset. So I suggest to introduce an argument return_original_indices_as_labels=True
which could be disabled. Or maybe better keep_point_labels=False
which, when set to True, preserves the labels of the input pointset instead of storing the point indices there. But please keep the behavior, it is used I think for the point clustering (could be checked)
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.
Perhaps instead of altering the input points, we could return a list of indices that we use to label as well?
The user can just as easily label them from the output (because PointSet.labels
is not a property). What do you think?
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.
I think you are right. I support this idea.
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.
As I implemented it, I changed my mind on this because we need to get the indices of the original PointSet after intersection
call and not necessarily after _points_inside
call.
So I think we should keep the current behavior since it creates a new PointSet anyway. (That is, while the return value is a reduced version of the original PointSet, it is still a new PointSet. As long as the docstring clearly states this, I think it is not a problem.)
…create PointSet from points
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.
I made some comments and two commits. Could you take a look?
…nts`) - update e2e/features/test_get.py ids
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #566 +/- ##
===========================================
+ Coverage 36.81% 53.52% +16.70%
===========================================
Files 61 69 +8
Lines 5421 6726 +1305
===========================================
+ Hits 1996 3600 +1604
+ Misses 3425 3126 -299 ☔ View full report in Codecov by Sentry. |
@@ -34,11 +35,46 @@ | |||
) | |||
|
|||
|
|||
def from_points(points: List["point.Point"], newlabels: List[int] = None) -> "PointSet": |
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.
Why wouldn't we just extend the PointSet constructor for this only slightly different variant?
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.
please see #566 (comment)
def __init__(self, coordinates, space=None, sigma_mm=0, labels: list = None): | ||
def __init__( | ||
self, | ||
coordinates: Union[List[Tuple], np.ndarray], |
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.
if we allow here a list of Point in addition to list of Tuple, we can make the space optional.
Not fully sure if this is more elegant, but to me it seems a bit confusing to have from_points although it is almost identical to the coordinate-based constructor
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.
It was previously possible to do so but during the last refactoring, if I remember correctly, this was removed. I wrote this method to continue with the same logic of volume.from_something()
.
Also, when the name of the keyword is coordinates
, it is not elegant to supply points that have more information.
@@ -90,11 +129,16 @@ def intersection(self, other: location.Location): | |||
return intersection[0] if len(intersection) == 1 else intersection | |||
|
|||
@property | |||
def sigma(self): | |||
return [p.sigma for p in self] | |||
def coordinates(self) -> np.ndarray: |
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.
is this only to protect modification of the coordinates?
Python is usually not too restrictive with attributes. Was there an issue leaving the coordinates editable? should be mentioned here in a comment.
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.
probably this makes sense, since modification of coordinates would have to maintain the sigma property as well.
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.
In the change log, it seems like sigma is edited out for coordinates but sigma has just moved below as a new property called coordinates
added. The reason is that the coordinates
used to be an attribute that is exposed and changeable, which potentially changes the data and makes a new point/pointset.
probably this makes sense, since modification of coordinates would have to maintain the sigma property as well.
Exactly. If the user would like to change the coordinate, then a new point or pointset should be created. This is important for keeping the original data as is.
siibra/volumes/volume.py
Outdated
|
||
# make sure the points are in the same physical space as this volume | ||
if isinstance(points, point.Point): # requires to be a PointSet from next step onwards | ||
as_pointset = pointset.from_points([points]) |
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.
I think I've seen a similar thing at other places.
I would find it more elegant to implement Point.as_pointlist() instead.
This would indicate that we are semantically casting to the list type rather than creating a copy.
What do you think?
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.
I thought about such a method as well but while PointSet inherits Points, the vice versa is not true. I am not sure it is a good design to create this cyclical dependency (especially since Point class should exist solely as it is a building block of PointSet IMO). Also, I do not think as_pointlist()
makes sense for a single point and I am not sure if there is use for users besides internal implementation.
warped = points.warp(self.space) | ||
assert warped is not None | ||
|
||
# transform the points to the voxel space of the volume for extracting values | ||
phys2vox = np.linalg.inv(img.affine) | ||
voxels = warped.transform(phys2vox, space=None) |
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.
I don't want to change anything here, but I repeatedly got across this space=None.
I think it is important not to have None as default so that the user realizes that we create a location outside the usual reference spaces.
However, the resulting space is not none, it is the pixel space of the image. In the future, we might want to create an id for the pixel space, and put that here.
This should go into a feature request for future discussion. no changes in this PR
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.
This is sth Xiao and I also have discussed; voxel space as a space should be possible. (Specifically, VoxelSpace under space.py. This would then point to a coordinate space still but requires information on how to get there, i.e. affine etc.) We plan to discuss this indeed
I think we should also consider using |
…outside_value param
5c0e498
to
2eb0f70
Compare
No description provided.