Add AnatomicalCoordinatesImage class #9
Conversation
h-mayorquin
left a comment
h-mayorquin
left a comment
There was a problem hiding this comment.
LGTM. Two suggestions.
- I think the representation is not obvious at glance. It confused me at the beginning. Something like this could help to clarify and illustrate:
Each pixel stores its complete (x, y, z) atlas coordinate:
Image with Atlas Coordinates per Pixel
j=0 j=1 j=2
──────────────────────────────────────────────
│ │ │ │
i=0 │ x: 2.10 │ x: 2.11 │ x: 2.12 │
│ y: -3.40 │ y: -3.40 │ y: -3.40 │
│ z: 1.20 │ z: 1.20 │ z: 1.20 │
│ │ │ │
├───────────────┼───────────────┼───────────────┤
│ │ │ │
i=1 │ x: 2.10 │ x: 2.11 │ x: 2.12 │
│ y: -3.41 │ y: -3.41 │ y: -3.41 │
│ z: 1.20 │ z: 1.20 │ z: 1.20 │
│ │ │ │
├───────────────┼───────────────┼───────────────┤
│ │ │ │
i=2 │ x: 2.10 │ x: 2.11 │ x: 2.12 │
│ y: -3.42 │ y: -3.42 │ y: -3.42 │
│ z: 1.20 │ z: 1.20 │ z: 1.20 │
│ │ │ │
───────────────────────────────────────────────
Each pixel stores its atlas coordinates (x, y, z)
x[i, j], y[i, j], z[i, j] give the atlas location for pixel (i, j)
- Also, maybe getting a function that gets you the coordinates per pixel could be useful as a help method on the class:
def get_atlas_coordinates(self, i=None, j=None):
import numpy as np
if i is not None and j is not None:
return (self.x[i, j], self.y[i, j], self.z[i, j])
else:
return np.stack([self.x[:], self.y[:], self.z[:]], axis=-1)The tests are not running, I am trying to fix that here:
#10
| quantity: 1 | ||
| doc: "The space in which the coordinates are defined" | ||
| name: space | ||
| - target_type: Image |
There was a problem hiding this comment.
I guess the schema does not have either or relationship, right? Or would itm make sense to have both Image and ImagingPlane?
There was a problem hiding this comment.
No, I don't think there's a way in the schema, but I can implement a check in the init of the class. I am not sure in which case could it be useful to have both Image and ImagingPlane.
| name: imaging_plane | ||
| attributes: | ||
| - name: method | ||
| doc: "The method used to determine the coordinates" |
There was a problem hiding this comment.
How standardized is this? should we really make it required? we should give some examples to illustrate what we intend here in the docs.
There was a problem hiding this comment.
I was replicating the AnatomicalCoordinatesTable attribute foe consistency, I am not sure what was the intent.
| - name: y | ||
| dtype: float32 | ||
| dims: | ||
| - - width |
There was a problem hiding this comment.
should we go the other way around here (height, width) thinking on the future?
Although I think that what they have is the orientation of the image.
There was a problem hiding this comment.
Exactly, Since it can be confusing in this case, I would keep it as the current NWB schema until we update it
| y = kwargs.get("y") | ||
| z = kwargs.get("z") | ||
| # verify that x, y, z have the same shape as the image data | ||
| if x.shape != image.data.shape or y.shape != image.data.shape or z.shape != image.data.shape: |
There was a problem hiding this comment.
is there an image_shape object that can fetch from the ImagingPlane to do a similar check? Maybe the grid?
There was a problem hiding this comment.
Mmh, no the grid_spacing only gives you the physical dimension of a pixel
|
|
||
| def test_create_custom_space(): | ||
| space = Space( | ||
| _ = Space( |
There was a problem hiding this comment.
It was a suggested ruff fix
Add
AnatomicalCoordinatesImageclass as described in #8