Add physical Coordinate support to SpotFindingResults #1516
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
10 tasks
Codecov Report
@@ Coverage Diff @@
## master #1516 +/- ##
==========================================
+ Coverage 87.27% 88.43% +1.15%
==========================================
Files 142 213 +71
Lines 5031 7971 +2940
==========================================
+ Hits 4391 7049 +2658
- Misses 640
8000
922 +282
Continue to review full report at Codecov.
|
This was referenced Aug 29, 2019
87cd345 to
a9f9535
Compare
18c85b6 to
29d66f5
Compare
ttung
requested changes
Sep 11, 2019
|
|
||
| pairs = ( | ||
| (Axes.X.value, Coordinates.X.value), | ||
| (Axes.Y.value, Coordinates.Y.value), | ||
| (Axes.ZPLANE.value, Coordinates.Z.value) | ||
| ) | ||
|
|
||
| if image_stack is None and spots is None: | ||
| raise ValueError("Must provide either SpotFindingResults or ImageStack to" |
There was a problem hiding this comment.
Suggested change
| raise ValueError("Must provide either SpotFindingResults or ImageStack to" | |
| raise ValueError("Must provide either SpotFindingResults or ImageStack to " |
| """ | ||
| Transfers physical coordinates from an Imagestack's coordinates xarray to an intensity table | ||
| Transfers physical coordinates from either an Imagestack or SpotFindingResults to an |
There was a problem hiding this comment.
Long term, will there be any workflows that extract physical coordinates from an ImageStack? Or is that a stopgap measure until SpotFindingResults is produced everywhere?
There was a problem hiding this comment.
Yea I think we'll be able to remove the imagestack option once the full refactors done
| @@ -12,7 +15,7 @@ class SpotFindingResults: | |||
| SpotAttributes. | |||
| """ | |||
|
|
|||
| def __init__(self, spot_attributes_list: Optional[List[Tuple]] = None): | |||
| def __init__(self, imagestack, spot_attributes_list: Optional[List[Tuple]] = None): | |||
There was a problem hiding this comment.
is it plausible that datasets get so big that we need to spread spot finding across multiple machines? If so, having the imagestack as part of spotfinding's results may make it challenging to persist.
It might make sense to store the coordinates from the imagestack, but not necessarily the actual imagestack (as that's really what you need anyway).
15c0db2 to
f60e4e3
Compare
29d66f5 to
9bf4e8e
Compare
f60e4e3 to
87178b9
Compare
9bf4e8e to
b5a67db
Compare
87178b9 to
b01a15a
Compare
b5a67db to
699ec22
Compare
b01a15a to
87af7ad
Compare
af7cd8a to
c27f333
Compare
741bb16 to
afecd9c
Compare
2f92ce6 to
71e65d9
Compare
71e65d9 to
5ae5c14
Compare
1ddb24f to
6145462
Compare
5ae5c14 to
0d800d8
Compare
6145462 to
49d8713
Compare
3664502 to
e9a2178
Compare
7d1f343 to
c033d9d
Compare
e9a2178 to
9147e3a
Compare
ttung
approved these changes
Sep 20, 2019
| def transfer_physical_coords_from_imagestack_to_intensity_table( | ||
| image_stack: ImageStack, intensity_table: IntensityTable | ||
| ) -> IntensityTable: | ||
| def transfer_physical_coords_to_intensity_table(*, intensity_table: IntensityTable, |
There was a problem hiding this comment.
may be cleaner as:
def transfer_physical_coords_to_intensity_table(
*,
intensity_table: IntensityTable,
image_stack: Optional[ImageStack] = None,
spots: Optional[SpotFindingResults] = None,
) -> IntensityTable:
"""
|
|
||
| pairs = ( | ||
| (Axes.X.value, Coordinates.X.value), | ||
| (Axes.Y.value, Coordinates.Y.value), | ||
| (Axes.ZPLANE.value, Coordinates.Z.value) | ||
| ) | ||
|
|
||
| if image_stack is None and spots is None: | ||
| raise ValueError("Must provide either SpotFindingResults or ImageStack to " |
There was a problem hiding this comment.
this is plausibly more wrapped than necessary.
| raise ValueError("Must provide either SpotFindingResults or ImageStack to " | ||
| "calculate coordinates from") | ||
|
|
||
| coord_ranges: Mapping[str, xr.DataArray] |
There was a problem hiding this comment.
As I discovered last night, the newest version of xarray will probably want this as:
Suggested change
| coord_ranges: Mapping[str, xr.DataArray] | |
| coord_ranges: Mapping[Hashable, xr.DataArray] |
|
|
||
| pairs = ( | ||
| (Axes.X.value, Coordinates.X.value), | ||
| (Axes.Y.value, Coordinates.Y.value), | ||
| (Axes.ZPLANE.value, Coordinates.Z.value) | ||
| ) | ||
|
|
||
| if image_stack is None and spots is None: |
There was a problem hiding this comment.
consider doing the xor. you really just want one specified. both is also bad.
c033d9d to
e495707
Compare
5589427 to
cbcdcb6
Compare
26f2f41 to
803d1a4
Compare
cbcdcb6 to
dae77b7
Compare
dae77b7 to
403b0bd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds physical coordinate information to the SpotResultsClass. This is needed for when the coordinates get transferred during decoding.
Depends on #1489 #1515
See it in action in #1518