-
Notifications
You must be signed in to change notification settings - Fork 68
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
ImageStack select on Physical Coordinates #1147
Conversation
@@ -383,14 +383,30 @@ def sel(self, indexers: Mapping[Axes, Union[int, tuple]]): | |||
ImageStack : | |||
a new image stack indexed by given value or range. | |||
""" | |||
|
|||
# convert indexers to Dict[str, (int/slice)] format | |||
# TODO shanaxel42 check if this can be changed to xarray.copy(deep=false) |
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.
Checked, cannot do regular copy get an xarray error.
Codecov Report
@@ Coverage Diff @@
## master #1147 +/- ##
==========================================
+ Coverage 88.41% 88.44% +0.03%
==========================================
Files 133 133
Lines 4953 4984 +31
==========================================
+ Hits 4379 4408 +29
- Misses 574 576 +2
Continue to review full report at Codecov.
|
assert iu.find_nearest(stack.xarray[Coordinates.X.value], -5) == 0 | ||
|
||
|
||
def test_convert_coords_to_indices(): |
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.
can we add a test that does both coordinate and index selection?
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.
added!
Codecov Report
@@ Coverage Diff @@
## master #1147 +/- ##
==========================================
- Coverage 89.08% 85.55% -3.53%
==========================================
Files 128 128
Lines 4891 4915 +24
==========================================
- Hits 4357 4205 -152
- Misses 534 710 +176
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1147 +/- ##
==========================================
+ Coverage 89.08% 89.09% +0.01%
==========================================
Files 128 128
Lines 4891 4915 +24
==========================================
+ Hits 4357 4379 +22
- Misses 534 536 +2
Continue to review full report at Codecov.
|
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 suspect this will break for images with noncontiguous labels. This is because you're getting the index offsets for the slice operation, but you are using .sel
. .sel
operates on labels.
At the very least, we should add a test to verify that noncontiguous labels + sel_by_physical_coords
works.
stack = deepcopy(self) | ||
selector = indexing_utils.convert_to_selector(indexers) | ||
stack._data._data = indexing_utils.index_keep_dimensions(self.xarray, selector) | ||
return stack | ||
|
||
def sel_by_physical_coords( | ||
self, indexers: Mapping[Coordinates, Union[Number, Tuple[Number, Number]]]): |
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 you find yourself repeating yourself a ton about Union[Number, Tuple[Number, Number]]
, you can create a type alias like how we created a type alias for Number
.
starfish/imagestack/imagestack.py
Outdated
|
||
Parameters | ||
---------- | ||
indexers : Dict[Axes, (float/tuple)]: |
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.
indexers : Dict[Axes, (float/tuple)]: | |
indexers : Mapping[Coordinates, Union[Number, Tuple[Number, Number]]]: |
---------- | ||
array: xr.DataArray | ||
The xarray with both physical and positional coordinates. | ||
indexers: Mapping[Coordinates, Union[int, tuple]] |
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.
indexers: Mapping[Coordinates, Union[int, tuple]] | |
indexers: Mapping[Coordinates, Union[Number, Tuple[Number, Number]]] |
data: xr.DataArray, indexers: Mapping[str, Union[int, slice]]) -> xr.DataArray: | ||
def convert_coords_to_indices(array: xr.DataArray, | ||
indexers: Mapping[Coordinates, Union[Number, Tuple[Number, Number]]] | ||
) -> Dict[Axes, Union[int, tuple]]: |
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.
) -> Dict[Axes, Union[int, tuple]]: | |
) -> Mapping[Axes, Union[float, Tuple[Number, Number]]]: |
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 method can't return a float since we're converting to positional indexers.
|
||
Returns | ||
------- | ||
Mapping[Axes, Union[int, tuple]]: |
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.
Mapping[Axes, Union[int, tuple]]: | |
Mapping[Axes, Union[float, Tuple[Number, Number]]]: |
|
||
Parameters | ||
---------- | ||
array: xr.DataArray |
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.
nit: @ambrosejcarr always wrote the parameters docs with a space before and after the colon, so I just copied him.
Wait why? I think this might only happen in the Z scenario. But since x/y are unlabeled sel and isel are effectively the same |
yes, I think you're right. |
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.
pls see comments, rest lgtm.
|
||
Parameters | ||
---------- | ||
indexers : Dict[Axes, (int/tuple)] |
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 inconsistent with the typing in your function declaration.
starfish/imagestack/imagestack.py
Outdated
>>> stack = ImageStack.synthetic_stack(5, 5, 15, 200, 200) | ||
>>> stack | ||
<starfish.ImageStack (r: 5, c: 5, z: 15, y: 200, x: 200)> | ||
>>> stack.sel({Axes.ROUND: (1, None), Axes.CH: 0, Axes.ZPLANE: 0}) |
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.
>>> stack.sel({Axes.ROUND: (1, None), Axes.CH: 0, Axes.ZPLANE: 0}) | |
>>> stack.isel({Axes.ROUND: (1, None), Axes.CH: 0, Axes.ZPLANE: 0}) |
starfish/imagestack/imagestack.py
Outdated
<starfish.ImageStack (r: 5, c: 5, z: 15, y: 200, x: 200)> | ||
>>> stack.sel({Axes.ROUND: (1, None), Axes.CH: 0, Axes.ZPLANE: 0}) | ||
<starfish.ImageStack (r: 4, c: 1, z: 1, y: 200, x: 200)> | ||
>>> stack.sel({Axes.ROUND: 0, Axes.CH: 0, Axes.ZPLANE: 1, |
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.
>>> stack.sel({Axes.ROUND: 0, Axes.CH: 0, Axes.ZPLANE: 1, | |
>>> stack.isel({Axes.ROUND: 0, Axes.CH: 0, Axes.ZPLANE: 1, |
data: xr.DataArray, indexers: Mapping[str, Union[int, slice]]) -> xr.DataArray: | ||
def convert_coords_to_indices(array: xr.DataArray, | ||
indexers: Mapping[Coordinates, Union[Number, Tuple[Number, Number]]] | ||
) -> Dict[Axes, Union[int, Tuple[Number, Number]]]: |
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.
) -> Dict[Axes, Union[int, Tuple[Number, Number]]]: | |
) -> Mapping[Axes, Union[int, Tuple[Number, Number]]]: |
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.
out of curiosity why do you prefer Mapping to Dict? Mapping always give me a lot of lint troubles
array: xr.DataArray | ||
The array to do lookups in. | ||
|
||
value: Union[float, tuple] |
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.
value: Union[float, tuple] | |
value : Union[Number, Tuple[Number, Number]] |
|
||
def find_nearest(array: xr.DataArray, | ||
value: Union[Number, Tuple[Number, Number]] | ||
) -> Union[int, Tuple[Number, Number]]: |
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.
) -> Union[int, Tuple[Number, Number]]: | |
) -> Union[int, Tuple[int, int]]: |
|
||
Returns | ||
------- | ||
Union[int, tuple]: |
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.
Union[int, tuple]: | |
Union[int, Tuple[int, int]]: |
Per the discussion in #1136 I think the correct path going forward is to maintain the xc/yc/zc coordinate arrays on our ImageStack object and provide both an Imagestck.sel_by_physical_coords() method and a convert_coords_to_indices() helper method to help index ImageStacks by physical coordinate values or ranges. This PR adds both and some tests.