10.) Formalizing the ability to return extra info from spot finding #1615

shanaxel42 · 2019-10-17T17:12:53Z

Changes the return value for every image_to_spots method to PerImageSpotResults, a named tuple that contains the SpotAttributes found in the image as well as any extra information for QC and debugging. The only spot finder that returns extra data at this point is PeakLocalMax which now returns threshold information.

fixes: #908, #708

codecov-io · 2019-10-17T17:30:33Z

Codecov Report

Merging #1615 into master will increase coverage by 42.12%.
The diff coverage is 97.56%.

@@             Coverage Diff             @@
##           master    #1615       +/-   ##
===========================================
+ Coverage   47.75%   89.87%   +42.12%     
===========================================
  Files         219      219               
  Lines        8117     8122        +5     
===========================================
+ Hits         3876     7300     +3424     
+ Misses       4241      822     -3419

Impacted Files	Coverage Δ
starfish/core/types/_constants.py	`98.24% <ø> (ø)`	⬆️
starfish/types.py	`100% <ø> (ø)`	⬆️
starfish/core/spots/DecodeSpots/trace_builders.py	`100% <100%> (+44.82%)`	⬆️
starfish/core/spots/DecodeSpots/util.py	`100% <100%> (+79.59%)`	⬆️
starfish/core/spots/FindSpots/blob.py	`96% <100%> (+6.2%)`	⬆️
starfish/core/types/_spot_finding_results.py	`97.61% <100%> (+13.4%)`	⬆️
...e/spots/FindSpots/trackpy_local_max_peak_finder.py	`98% <100%> (+40%)`	⬆️
...tarfish/core/spots/FindSpots/spot_finding_utils.py	`100% <100%> (+5.88%)`	⬆️
starfish/core/types/__init__.py	`100% <100%> (ø)`	⬆️
...fish/core/spots/FindSpots/local_max_peak_finder.py	`74.03% <90%> (+43.94%)`	⬆️
... and 158 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 658ff1e...ce9e2d0. Read the comment docs.

ttung · 2019-10-25T18:30:15Z

starfish/core/spots/DecodeSpots/trace_builders.py

@@ -68,7 +73,7 @@ def build_traces_sequential(spot_results: SpotFindingResults, **kwargs) -> Inten

    i = 0
    for (r, c), attrs in spot_results.items():


would not call this attrs in this context because attrs and spot_attrs gets confusing.

ttung · 2019-10-25T18:30:29Z

starfish/core/spots/FindSpots/blob.py

@@ -84,7 +85,7 @@ def __init__(
        except ValueError:
            raise ValueError("Detector method must be one of {blob_log, blob_dog, blob_doh}")

-    def image_to_spots(self, data_image: Union[np.ndarray, xr.DataArray]) -> SpotAttributes:
+    def image_to_spots(self, data_image: Union[np.ndarray, xr.DataArray]) -> PerImageSpotResults:


PerTileSpotResults?

Would we call it tile though since we don't include a z value? I went back a forth about this but all your TileKey TileFetcher code includes a r/ch/z for each tile so I wasn't sure it was the same

PerImageSliceSpotResults?

fwiw, I thought we should call n-dimensional tensors images (plural) rather than singular, which I think would have avoided this wonkiness. You should consult with the proponents of calling the entire n-dimensional tensor "image". :)

If they say nothing, then I would go with your judgement.

ttung · 2019-10-25T18:39:13Z

starfish/core/types/_constants.py

@@ -40,6 +41,13 @@ class Coordinates(AugmentedEnum):
 """


+PerImageSpotResults = namedtuple('PerImageSpotResults', 'spot_attrs, extras')


Would make this a dataclass.

ttung · 2019-10-25T18:39:46Z

starfish/core/types/_constants.py

@@ -40,6 +41,13 @@ class Coordinates(AugmentedEnum):
 """


+PerImageSpotResults = namedtuple('PerImageSpotResults', 'spot_attrs, extras')
+"""
+Named tuple that gets returned in every spot finding"s image_to_spots method, spot_attrs are the


Suggested change

Named tuple that gets returned in every spot finding"s image_to_spots method, spot_attrs are the

Named tuple that gets returned in every spot finding's image_to_spots method, spot_attrs are the

ttung · 2019-10-25T18:40:16Z

starfish/core/types/_constants.py

@@ -40,6 +41,13 @@ class Coordinates(AugmentedEnum):
 """


+PerImageSpotResults = namedtuple('PerImageSpotResults', 'spot_attrs, extras')


wait is this right? this is just two strings.

The strings are just the names of the values put into the named tuple. So you can access them like PerImageSpotResults.spot_attrs

Shouldn't it be:

Suggested change

PerImageSpotResults = namedtuple('PerImageSpotResults', 'spot_attrs, extras')

PerImageSpotResults = namedtuple('PerImageSpotResults', 'spot_attrs', 'extras')

ttung · 2019-10-25T18:45:48Z

starfish/core/spots/FindSpots/h_max.py

@@ -11,7 +11,8 @@

 from starfish.core.imagestack.imagestack import ImageStack
 from starfish.core.spots.FindSpots import spot_finding_utils
-from starfish.core.types import Axes, Features, SpotAttributes, SpotFindingResults
+from starfish.core.types import Axes, Features, PerImageSpotResults, SpotAttributes, \


i would wrap this like how you did it for starfish/core/spots/DecodeSpots/trace_builders.py

ttung

see comments pls.

ttung · 2019-11-06T22:46:08Z

starfish/core/spots/FindSpots/local_max_peak_finder.py

@@ -13,7 +13,8 @@
 from starfish.core.config import StarfishConfig
 from starfish.core.imagestack.imagestack import ImageStack
 from starfish.core.spots.FindSpots import spot_finding_utils
-from starfish.core.types import Axes, Features, Number, SpotAttributes, SpotFindingResults
+from starfish.core.types import Axes, Features, Number, PerImageSliceSpotResults, SpotAttributes, \


this can be wrapped without \

ttung · 2019-11-06T22:46:36Z

starfish/core/spots/FindSpots/local_max_peak_finder.py

@@ -173,7 +174,8 @@ def _select_optimal_threshold(self, thresholds: np.ndarray, spot_counts: List[in

        return selected_thr

-    def _compute_threshold(self, img: Union[np.ndarray, xr.DataArray]) -> float:
+    def _compute_threshold(self, img: Union[np.ndarray, xr.DataArray]
+                           ) -> Tuple[float, Optional[np.ndarray], Optional[List[int]]]:


can you update the docblock for this?

shanaxel42 mentioned this pull request Oct 17, 2019

Implement SpotFinding Refactor #1514

Closed

10 tasks

shanaxel42 force-pushed the saxelrod-spot-results-extras branch 3 times, most recently from 992dd01 to 984644d Compare October 17, 2019 17:24

shanaxel42 changed the title ~~Formalizing the ability to return extra info from spot finding~~ 10.) Formalizing the ability to return extra info from spot finding Oct 17, 2019

shanaxel42 requested review from ttung and ambrosejcarr October 17, 2019 17:24

shanaxel42 force-pushed the saxelrod-graph-decoding branch from f2cc8cf to 2879c3c Compare October 17, 2019 20:24

shanaxel42 force-pushed the saxelrod-spot-results-extras branch 2 times, most recently from b42b879 to fa0296b Compare October 18, 2019 20:51

shanaxel42 force-pushed the saxelrod-graph-decoding branch from 431ad13 to 9ac3b89 Compare October 18, 2019 21:38

shanaxel42 force-pushed the saxelrod-spot-results-extras branch 2 times, most recently from 757bbc3 to 1cc1e36 Compare October 21, 2019 17:26

ttung requested changes Oct 25, 2019

View reviewed changes

ttung reviewed Oct 25, 2019

View reviewed changes

shanaxel42 force-pushed the saxelrod-spot-results-extras branch 3 times, most recently from 67d16b2 to 9580ccd Compare November 6, 2019 17:53

shanaxel42 requested a review from ttung November 6, 2019 17:53

ttung approved these changes Nov 6, 2019

View reviewed changes

shanaxel42 force-pushed the saxelrod-spot-results-extras branch 2 times, most recently from edf3226 to af812f7 Compare November 12, 2019 17:59

shanaxel42 changed the base branch from saxelrod-graph-decoding to master November 12, 2019 19:16

adding an extras feild to SpotFindingResults

ce9e2d0

shanaxel42 force-pushed the saxelrod-spot-results-extras branch from af812f7 to ce9e2d0 Compare November 12, 2019 22:50

shanaxel42 merged commit 4afc114 into master Nov 13, 2019

shanaxel42 deleted the saxelrod-spot-results-extras branch November 13, 2019 00:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

10.) Formalizing the ability to return extra info from spot finding #1615

10.) Formalizing the ability to return extra info from spot finding #1615

shanaxel42 commented Oct 17, 2019 •

edited

Loading

codecov-io commented Oct 17, 2019 •

edited

Loading

ttung Oct 25, 2019

ttung Oct 25, 2019

shanaxel42 Nov 4, 2019

ttung Nov 4, 2019

ttung Oct 25, 2019

ttung Oct 25, 2019

ttung Oct 25, 2019

shanaxel42 Nov 4, 2019

ttung Nov 4, 2019

ttung Oct 25, 2019

ttung left a comment

ttung Nov 6, 2019

ttung Nov 6, 2019

		@@ -68,7 +73,7 @@ def build_traces_sequential(spot_results: SpotFindingResults, **kwargs) -> Inten

		i = 0
		for (r, c), attrs in spot_results.items():

		@@ -40,6 +41,13 @@ class Coordinates(AugmentedEnum):
		"""


		PerImageSpotResults = namedtuple('PerImageSpotResults', 'spot_attrs, extras')

	Named tuple that gets returned in every spot finding"s image_to_spots method, spot_attrs are the
	Named tuple that gets returned in every spot finding's image_to_spots method, spot_attrs are the

10.) Formalizing the ability to return extra info from spot finding #1615

10.) Formalizing the ability to return extra info from spot finding #1615

Conversation

shanaxel42 commented Oct 17, 2019 • edited Loading

codecov-io commented Oct 17, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanaxel42 commented Oct 17, 2019 •

edited

Loading

codecov-io commented Oct 17, 2019 •

edited

Loading