Adding new modules and data SpotFindingResults class. #1515

shanaxel42 · 2019-08-29T07:08:34Z

This PR lays some of the ground work for the spot finding refactor.

Adds two new modules FindSpots and DecodeSpots (which will be the only ones left by the end of this).
Adds their base class api contracts.
Adds new class SpotFindingResults which is a wrapper class around a dictionary of round/ch index pairs and the spots found there (SpotAttributes).

Depends on #1489

codecov-io · 2019-08-29T07:17:04Z

Codecov Report

Merging #1515 into master will decrease coverage by 0.92%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1515      +/-   ##
==========================================
- Coverage   88.18%   87.26%   -0.93%     
==========================================
  Files         138      139       +1     
  Lines        4978     5026      +48     
==========================================
- Hits         4390     4386       -4     
- Misses        588      640      +52

Impacted Files	Coverage Δ
starfish/core/__init__.py	`87.5% <0%> (-12.5%)`	⬇️
starfish/core/codebook/codebook.py	`96.62% <0%> (-0.03%)`	⬇️
starfish/core/imagestack/imagestack.py	`94.42% <0%> (-0.02%)`	⬇️
...ore/intensity_table/intensity_table_coordinates.py	`100% <0%> (ø)`	⬆️
starfish/core/util/logging.py	`96.66% <0%> (ø)`	⬆️
starfish/core/util/dtype.py	`100% <0%> (ø)`	⬆️
starfish/core/spots/DetectSpots/_base.py	`81.81% <0%> (ø)`	⬆️
starfish/core/imagestack/_mp_dataarray.py	`0% <0%> (ø)`
...re/spots/DetectPixels/combine_adjacent_features.py	`98.18% <0%> (+0.01%)`	⬆️

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 f4c63a8...803d1a4. Read the comment docs.

ttung

Since this is a new class, it may be helpful to consider which APIs you want end users (i.e., pipeline users) to be using, and prefixing the other APIs with _.

ttung · 2019-08-29T17:24:05Z

starfish/core/types/_spot_finding_resutls.py

+        Parameters
+        -----------
+        spot_attributes_list : Optional[List[Tuple[indices, SpotAttributes]]]
+            If spots were fond using Imagestack.transform() the result is a list of


Suggested change

If spots were fond using Imagestack.transform() the result is a list of

If spots were found using ImageStack.transform() the result is a list of

ttung · 2019-08-29T17:24:55Z

starfish/core/types/_spot_finding_resutls.py

+
+class SpotFindingResults:
+    """
+    Wrapper class that describes the results from a spot finding method. The


is your fill column set to 80 and not 100?

ttung · 2019-08-29T17:27:29Z

starfish/core/types/_spot_finding_resutls.py

+class SpotFindingResults:
+    """
+    Wrapper class that describes the results from a spot finding method. The
+    results dict is a collection of (round,ch) indices and their corresponding measured


Suggested change

results dict is a collection of (round,ch) indices and their corresponding measured

results dict is a collection of (round, ch) indices and their corresponding measured

(because you used a space below in your other tuple)

ttung · 2019-08-29T17:28:04Z

starfish/core/types/_spot_finding_resutls.py

+class SpotFindingResults:
+    """
+    Wrapper class that describes the results from a spot finding method. The
+    results dict is a collection of (round,ch) indices and their corresponding measured


Is this API doc likely to be read by an end user? Would results dict be confusing?

ttung · 2019-08-29T22:01:39Z

starfish/core/types/_spot_finding_resutls.py

+            tuples (indices, SpotAttributes). Instantiating SpotFindingResults with
+            this list will convert the information to a dictionary.
+        """
+        self._results: Mapping[Tuple, SpotAttributes] = dict()


This could be written as:

spot_attributes_list = spot_attributes_list or [] self._results: Mapping[Tuple, SpotAttributes] = { indices: spots for indices, spots in spot_attributes_list }

ttung · 2019-08-30T17:03:19Z

starfish/core/types/_spot_finding_resutls.py

+            for indices, spots in spot_attributes_list:
+                self._results[indices] = spots
+
+    def set_spots_for_round_ch(self, indices: Mapping[Axes, int], spots: SpotAttributes


nit: this function declaration style leads to unnecessary LOC changes. would prefer:

def set_spots_for_round_ch(self, indices: Mapping[Axes, int], spots: SpotAttributes) -> None:

or

def set_spots_for_round_ch( self, indices: Mapping[Axes, int], spots: SpotAttributes) -> None:

or

def set_spots_for_round_ch( self, indices: Mapping[Axes, int], spots: SpotAttributes, ) -> None:

All of these approaches do not produce unnecessary changes if the method name changes.

ttung · 2019-08-30T17:04:10Z

starfish/core/types/_spot_finding_resutls.py

+            Describes spots found on this tile.
+        """
+        round_ch_index = tuple(indices[i] for i in AXES_ORDER)
+        self._results[round_ch_index] = spots  # type: ignore


You can get rid of the # type: ignore if you make self._results a MutableMapping.

ttung · 2019-08-30T17:05:20Z

starfish/core/types/_spot_finding_resutls.py

+    def set_spots_for_round_ch(self, indices: Mapping[Axes, int], spots: SpotAttributes
+                               ) -> None:
+        """
+        Add the r,ch indices and corresponding SpotAttributes to the results dict.


Suggested change

Add the r,ch indices and corresponding SpotAttributes to the results dict.

Add the round, ch indices and corresponding SpotAttributes to the results dict.

to match what you are doing elsewhere.

ttung · 2019-08-30T17:06:27Z

starfish/core/types/_spot_finding_resutls.py

+        """
+        Return the set of Round labels in the SpotFindingResults
+        """
+        return list(set(sorted(r for (r, ch) in self.round_ch_indices())))


if you take a set of sorted, then you lose the sorting. did you want sorted(set(...))?

ttung · 2019-08-30T17:06:32Z

starfish/core/types/_spot_finding_resutls.py

+        """
+        Return the set of ch labels in the SpotFindingResults
+        """
+        return list(set(sorted(ch for (c, ch) in self.round_ch_indices())))


ttung · 2019-09-11T16:57:18Z

starfish/core/types/__init__.py

@@ -15,6 +15,7 @@
 )
 from ._decoded_spots import DecodedSpots
 from ._spot_attributes import SpotAttributes
+from ._spot_finding_resutls import SpotFindingResults


Suggested change

from ._spot_finding_resutls import SpotFindingResults

from ._spot_finding_results import SpotFindingResults

(and as such, the file probably needs to be renamed)

ambrosejcarr · 2019-09-13T15:52:58Z

starfish/core/spots/FindSpots/_base.py

+    @abstractmethod
+    def run(self, image_stack: ImageStack,
+            reference_image: Optional[ImageStack] = None, *args) -> SpotFindingResults:
+        """Find and measure spots across rounds and chs in the provided ImageStack."""


Suggested change

"""Find and measure spots across rounds and chs in the provided ImageStack."""

"""Find and measure spots across rounds and channels in the provided ImageStack."""

ambrosejcarr · 2019-09-13T16:02:56Z

starfish/core/types/_spot_finding_resutls.py

+AXES_ORDER = (Axes.ROUND, Axes.CH)
+
+
+class SpotFindingResults:


Given this wraps a dict, I would prefer this be implemented to mimic the dictionary container, and more clearly document what it contains. This should simplify a lot of the accessors you've written. For example,

+ def values(self): - def all_spots(self): - """ - Return all SpotAttributes across rounds and chs. - """ return self._results.values()

See: https://docs.python.org/2.5/ref/sequence-types.html

Yea so I started down this route but found that things became too confusing. For example get_spots_for_round_ch would instead just be getitem(key) and then when you used it it wasn't totally clear what you were getting. I'm not super opinionated on this though so open to changing it back if that's what you'd prefer I just thought this was more descriptive

starfish/core/types/_spot_finding_resutls.py

ttung · 2019-09-17T00:19:32Z

starfish/core/types/_spot_finding_resutls.py

+        """
+        Return the set of ch labels in the SpotFindingResults
+        """
+        return list(sorted(set(ch for (c, ch) in self.round_ch_indices())))


Suggested change

return list(sorted(set(ch for (c, ch) in self.round_ch_indices())))

return list(sorted(set(ch for (r, ch) in self.round_ch_indices())))

ttung

I don't think this will doc correctly.

ttung · 2019-09-20T00:30:09Z

starfish/core/types/_spot_finding_results.py

+        -----------
+        spot_attributes_list : Optional[List[Tuple]]
+            If spots were found using ImageStack.transform() the result is a list of
+            tuples (indices, SpotAttributes). Instantiating SpotFindingResults with


The format for the indices is not very clear. It appears to be a Tuple of index values in the order of AXES_ORDER, but I'm just inferring that from the code.

That's exactly what it is! I'll make it more clear

ttung · 2019-09-20T00:33:16Z

starfish/core/types/_spot_finding_results.py

+        """
+        Return the set of Round labels in the SpotFindingResults
+        """
+        return list(sorted(set(r for (r, ch) in self.keys())))


i still think sorted(set(..)) would do what you want. the extra list() shouldn't be necessary.

>>> sorted(set([1, 2, 3, 4])) [1, 2, 3, 4] >>> ^D

ttung · 2019-09-20T00:33:37Z

starfish/core/types/_spot_finding_results.py

+    @property
+    def round_labels(self):
+        """
+        Return the set of Round labels in the SpotFindingResults


Suggested change

Return the set of Round labels in the SpotFindingResults

Return the set of round labels in the SpotFindingResults

ttung

Just a few docs and comments issues, rest lgtm.

ttung · 2019-09-23T17:21:02Z

docs/source/api/spots/index.rst

+
+    from starfish.spots import FindSpots
+
+.. autoclass:: starfish.spots.FindSpots


Isn't FindSpots a module? I think you want automodule here to recursively pull in all the implementations in FindSpots, and the weird logic to import in all the submodules of FindSpots.

ttung · 2019-09-23T17:21:09Z

docs/source/api/spots/index.rst

+
+.. code-block:: python
+
+    from starfish.spots import DecodeSpots


ttung · 2019-09-23T17:22:04Z

starfish/core/spots/FindSpots/_base.py

+    @abstractmethod
+    def run(self, image_stack: ImageStack,
+            reference_image: Optional[ImageStack] = None, *args) -> SpotFindingResults:
+        """Find and measure spots across rounds and channels in the provided


unwrap? I'd check your editor to make sure you're set up to 100col. This clocks in at 92col unwrapped for me.

shanaxel42 mentioned this pull request Aug 29, 2019

Implement SpotFinding Refactor #1514

Closed

10 tasks

This was referenced Aug 29, 2019

Add physical Coordinate support to SpotFindingResults #1516

Merged

1.) Provenance logging refactor and support for SpotFindingResults #1517

Merged

2) ISS spot finding refactor #1518

Merged

shanaxel42 requested review from ttung and ambrosejcarr August 29, 2019 16:59

ttung requested changes Aug 30, 2019

View reviewed changes

ttung reviewed Sep 11, 2019

View reviewed changes

ambrosejcarr requested changes Sep 13, 2019

View reviewed changes

ttung reviewed Sep 17, 2019

View reviewed changes

starfish/core/types/_spot_finding_resutls.py Outdated Show resolved Hide resolved

ttung reviewed Sep 17, 2019

View reviewed changes

shanaxel42 force-pushed the saxelrod-decoded-intensity-table branch from 1add80c to ae65a33 Compare September 17, 2019 21:41

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from 15c0db2 to f60e4e3 Compare September 17, 2019 21:49

shanaxel42 force-pushed the saxelrod-decoded-intensity-table branch from ae65a33 to 66481de Compare September 17, 2019 23:40

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from f60e4e3 to 87178b9 Compare September 17, 2019 23:42

shanaxel42 force-pushed the saxelrod-decoded-intensity-table branch from 66481de to d6c8012 Compare September 18, 2019 18:49

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch 2 times, most recently from b01a15a to 87af7ad Compare September 18, 2019 19:30

shanaxel42 force-pushed the saxelrod-decoded-intensity-table branch from d6c8012 to 667ebb1 Compare September 18, 2019 20:44

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from 87af7ad to 1308d03 Compare September 18, 2019 20:44

shanaxel42 force-pushed the saxelrod-decoded-intensity-table branch from 667ebb1 to cc5a4c7 Compare September 18, 2019 21:04

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch 2 times, most recently from 741bb16 to afecd9c Compare September 18, 2019 21:09

shanaxel42 force-pushed the saxelrod-decoded-intensity-table branch 2 times, most recently from 5db69cb to dd52d49 Compare September 19, 2019 17:58

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from afecd9c to 5b11569 Compare September 19, 2019 17:58

shanaxel42 requested a review from ttung September 19, 2019 18:01

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from 5b11569 to 1ddb24f Compare September 19, 2019 21:09

shanaxel42 force-pushed the saxelrod-decoded-intensity-table branch from dd52d49 to 7fce7e4 Compare September 19, 2019 21:49

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from 1ddb24f to 6145462 Compare September 19, 2019 21:49

shanaxel42 force-pushed the saxelrod-decoded-intensity-table branch from 7fce7e4 to a112655 Compare September 19, 2019 22:12

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from 6145462 to 49d8713 Compare September 19, 2019 22:12

shanaxel42 force-pushed the saxelrod-decoded-intensity-table branch from a112655 to b73ab2f Compare September 19, 2019 23:03

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from 49d8713 to 7d1f343 Compare September 19, 2019 23:03

shanaxel42 changed the base branch from saxelrod-decoded-intensity-table to master September 19, 2019 23:36

shanaxel42 changed the base branch from master to saxelrod-decoded-intensity-table September 19, 2019 23:38

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from 7d1f343 to c033d9d Compare September 19, 2019 23:42

shanaxel42 changed the base branch from saxelrod-decoded-intensity-table to master September 19, 2019 23:51

shanaxel42 requested review from ttung and removed request for ttung September 19, 2019 23:52

ttung requested changes Sep 20, 2019

View reviewed changes

ttung reviewed Sep 20, 2019

View reviewed changes

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from c033d9d to e495707 Compare September 20, 2019 15:13

shanaxel42 requested a review from ttung September 20, 2019 15:14

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from e495707 to 26f2f41 Compare September 20, 2019 20:59

ttung requested changes Sep 23, 2019

View reviewed changes

adding new SpotFindingResults data structure and new packages

803d1a4

shanaxel42 force-pushed the saxelrod-new-spot-finding-data-structures branch from 26f2f41 to 803d1a4 Compare September 23, 2019 18:06

shanaxel42 merged commit ac5ea3a into master Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new modules and data SpotFindingResults class. #1515

Adding new modules and data SpotFindingResults class. #1515

shanaxel42 commented Aug 29, 2019 •

edited

Loading

codecov-io commented Aug 29, 2019 •

edited

Loading

ttung left a comment

ttung Aug 29, 2019

ttung Aug 29, 2019

ttung Aug 29, 2019

ttung Aug 29, 2019

ttung Aug 29, 2019

ttung Aug 30, 2019

ttung Aug 30, 2019

ttung Aug 30, 2019

ttung Aug 30, 2019

ttung Aug 30, 2019

ttung Sep 11, 2019

ambrosejcarr Sep 13, 2019

ambrosejcarr Sep 13, 2019

shanaxel42 Sep 18, 2019 •

edited

Loading

ttung Sep 17, 2019

ttung left a comment

ttung Sep 20, 2019

shanaxel42 Sep 20, 2019

ttung Sep 20, 2019

ttung Sep 20, 2019

ttung left a comment

ttung Sep 23, 2019

ttung Sep 23, 2019

ttung Sep 23, 2019

	If spots were fond using Imagestack.transform() the result is a list of
	If spots were found using ImageStack.transform() the result is a list of

	results dict is a collection of (round,ch) indices and their corresponding measured
	results dict is a collection of (round, ch) indices and their corresponding measured

	Add the r,ch indices and corresponding SpotAttributes to the results dict.
	Add the round, ch indices and corresponding SpotAttributes to the results dict.

	from ._spot_finding_resutls import SpotFindingResults
	from ._spot_finding_results import SpotFindingResults

	"""Find and measure spots across rounds and chs in the provided ImageStack."""
	"""Find and measure spots across rounds and channels in the provided ImageStack."""

	return list(sorted(set(ch for (c, ch) in self.round_ch_indices())))
	return list(sorted(set(ch for (r, ch) in self.round_ch_indices())))

	Return the set of Round labels in the SpotFindingResults
	Return the set of round labels in the SpotFindingResults


		from starfish.spots import FindSpots

		.. autoclass:: starfish.spots.FindSpots


		.. code-block:: python

		from starfish.spots import DecodeSpots

Adding new modules and data SpotFindingResults class. #1515

Adding new modules and data SpotFindingResults class. #1515

Conversation

shanaxel42 commented Aug 29, 2019 • edited Loading

codecov-io commented Aug 29, 2019 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

shanaxel42 Sep 18, 2019 • edited Loading

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

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

Choose a reason for hiding this comment

shanaxel42 commented Aug 29, 2019 •

edited

Loading

codecov-io commented Aug 29, 2019 •

edited

Loading

shanaxel42 Sep 18, 2019 •

edited

Loading