Conversation
jenna-tomkinson
left a comment
jenna-tomkinson
left a comment
There was a problem hiding this comment.
This PR is good to go, but I would highly recommend that you go over the naming conventions in this feature measurement and add code comments to provide more clarity. Excited to see ZedProfiler come alive! :D
| @@ -1,10 +1,168 @@ | |||
| """Area/size/shape featurization module scaffold.""" | |||
| """Area, size, and shape features for 3D objects.""" | |||
There was a problem hiding this comment.
Have you considered renaming to Volume or Surface area to be more specific towards 3D featurization? This is more of a nit-pick as it is pretty understandable with the current naming.
| max(props["bbox-1"][0], 0) : min(props["bbox-4"][0], label_object.shape[1]), | ||
| max(props["bbox-2"][0], 0) : min(props["bbox-5"][0], label_object.shape[2]), | ||
| ] | ||
| volume_truths = volume > 0 |
There was a problem hiding this comment.
Is this here to avoid any object with a volume = 0?
| features_to_record = _empty_feature_result() | ||
|
|
||
| desired_properties = [ | ||
| "area", |
There was a problem hiding this comment.
Isn't area not a 3D feature? Should this be volume?
| "equivalent_diameter", | ||
| ] | ||
| for label in unique_objects: | ||
| if label == 0: |
There was a problem hiding this comment.
Is this label ID or what is 0 in this case?
| subset_lab_object = label_object.copy() | ||
| subset_lab_object[subset_lab_object != label] = 0 |
There was a problem hiding this comment.
I am a bit confused by subset_lab_object. I can see it is a copy of label_object, so what makes it a subset in this case? Also, should it be subet_label_object?
| ) | ||
|
|
||
| features_to_record["object_id"].append(label) | ||
| features_to_record["Volume"].append(props["area"].item()) |
There was a problem hiding this comment.
This might get a bit confusing to interchange volume and area here. Recommend sticking to one convention if possible.
There was a problem hiding this comment.
Overall, this code is very clean! Though, I highly recommend more code comments here. There are instances where area and volume are used interchangeably, which is not always clear, and I find the section on labels unclear about why we are avoiding 0 as a label (especially if it is 0-indexed).
| class SupportsImageSetLoader(Protocol): | ||
| """Minimal image-set loader interface required by this module.""" | ||
|
|
||
| anisotropy_spacing: tuple[float, float, float] |
There was a problem hiding this comment.
Recommend adding comment here on what anisotropy spacing is. Someone new to 3D might not have knowledge of this already and it is probably safer to be more explicit here.
| anisotropy_spacing: tuple[float, float, float] | |
| # Pixel size in x,y,z space? | |
| anisotropy_spacing: tuple[float, float, float] |
There was a problem hiding this comment.
Also, is this loading in an image-set or is this setting the spacing distances for 3D? The name SupportsImageSetLoader makes it sound like it is taking in the image path and loading it in without any other context.
| class SupportsObjectLoader(Protocol): | ||
| """Minimal object loader interface required by this module.""" | ||
|
|
||
| label_image: np.ndarray | ||
| object_ids: Sequence[int] | ||
|
|
There was a problem hiding this comment.
What is label_image? As an array that sounds like it will be the FOV since we usually call FOVs images, but for AreaShape that FOV only matters for segmentation. In this case, is label_image actually the label_mask from the segmentation module?
There was a problem hiding this comment.
Is this supposed to be in this PR?
jenna-tomkinson
left a comment
jenna-tomkinson
left a comment
There was a problem hiding this comment.
Apologises I meant to approve before!
Description
Adding and implementing the areasizeshape module
What kind of change(s) are included?
Checklist
Please ensure that all boxes are checked before indicating that this pull request is ready for review.