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

Add Volume.evaluate_points() and fix voxel access for _points_inside #566

Merged
merged 9 commits into from
Mar 11, 2024

Conversation

dickscheid
Copy link
Contributor

No description provided.

…uate_points
"""
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.
Copy link
Contributor Author

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)

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

@AhmetNSimsek AhmetNSimsek Feb 27, 2024

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.)

Copy link
Collaborator

@AhmetNSimsek AhmetNSimsek left a 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-commenter
Copy link

Codecov Report

Attention: Patch coverage is 62.50000% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 53.52%. Comparing base (5a0e6b3) to head (b83aa45).
Report is 424 commits behind head on main.

Files Patch % Lines
siibra/volumes/volume.py 63.63% 12 Missing ⚠️
siibra/livequeries/bigbrain.py 0.00% 10 Missing ⚠️
siibra/core/region.py 0.00% 5 Missing ⚠️
...bra/features/tabular/bigbrain_intensity_profile.py 0.00% 2 Missing ⚠️
siibra/locations/__init__.py 50.00% 1 Missing ⚠️

❗ 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.
📢 Have feedback on the report? Share it here.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@@ -34,11 +35,46 @@
)


def from_points(points: List["point.Point"], newlabels: List[int] = None) -> "PointSet":
Copy link
Contributor Author

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?

Copy link
Collaborator

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],
Copy link
Contributor Author

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

Copy link
Collaborator

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:
Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

@AhmetNSimsek AhmetNSimsek Mar 10, 2024

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.


# 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])
Copy link
Contributor Author

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?

Copy link
Collaborator

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)
Copy link
Contributor Author

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

Copy link
Collaborator

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

@AhmetNSimsek
Copy link
Collaborator

I think we should also consider using evaluate_points instead of parcelationmap.Map._read_voxel

…outside_value param
@AhmetNSimsek AhmetNSimsek force-pushed the enh_points_inside_volume branch from 5c0e498 to 2eb0f70 Compare March 11, 2024 15:43
@AhmetNSimsek AhmetNSimsek merged commit 36da8c9 into main Mar 11, 2024
27 checks passed
@AhmetNSimsek AhmetNSimsek deleted the enh_points_inside_volume branch March 11, 2024 16:52
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