Adding DecodedIntensityTable class #1489

shanaxel42 · 2019-08-13T17:47:38Z

Introducing DecodedIntensityTable which is just a subclass of IntensityTable. I moved anything related to decoded spots into the DecodedIntensityTable class. This is mostly to reduce confusion in the codebase between the outputs of various spot finding and decoding methods. In most cases the very end of a pipeline will now be a DecodedIntensityTable.

codecov-io · 2019-08-13T17:59:08Z

Codecov Report

Merging #1489 into master will decrease coverage by 0.06%.
The diff coverage is 98.87%.

@@            Coverage Diff             @@
##           master    #1489      +/-   ##
==========================================
- Coverage   87.23%   87.17%   -0.07%     
==========================================
  Files         138      140       +2     
  Lines        5023     5020       -3     
==========================================
- Hits         4382     4376       -6     
- Misses        641      644       +3

Impacted Files	Coverage Δ
starfish/core/intensity_table/intensity_table.py	`96.26% <ø> (-0.89%)`	⬇️
starfish/core/spots/Decode/metric_decoder.py	`100% <100%> (ø)`	⬆️
starfish/core/codebook/codebook.py	`96.62% <100%> (-0.24%)`	⬇️
starfish/__init__.py	`100% <100%> (ø)`	⬆️
starfish/core/spots/AssignTargets/label.py	`96% <100%> (+0.16%)`	⬆️
...core/spots/Decode/per_round_max_channel_decoder.py	`100% <100%> (ø)`	⬆️
...sh/core/intensity_table/decoded_intensity_table.py	`98.61% <98.61%> (ø)`
starfish/core/image/Filter/linear_unmixing.py	`73.07% <0%> (-26.93%)`	⬇️
...sh/core/spots/DetectSpots/local_max_peak_finder.py	`48.07% <0%> (-21.4%)`	⬇️
...re/spots/DetectPixels/combine_adjacent_features.py	`84.54% <0%> (-13.64%)`	⬇️
... and 24 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 c16c8de...b73ab2f. Read the comment docs.

ttung

Mostly fine. The move of the plausibly-only-used-in-test-code code to its own module can happen in a separate PR if easier. The deduplication of synthetic_intensities should happen.

ttung · 2019-08-29T17:06:42Z

starfish/core/codebook/codebook.py

+            return DecodedIntensityTable.from_intensity_table(
+                intensities,
+                targets=(Features.AXIS, np.empty(0, dtype='U')),
+                distances=(Features.AXIS, np.empty(0, dtype=float)),


nit: would prefer the explicit type, like np.float32

Fine with this suggestion but suggest not dropping precision: suggest using np.float64 if changing.

ttung · 2019-08-29T17:12:26Z

starfish/core/intensity_table/decoded_intensity_table.py

+
+        """
+
+        # TODO nsofroniew: right now there is no jitter on x-y positions of the spots


IMO, this should probably call IntensityTable.synthetic_intensities and assign targets to the spots.

ttung · 2019-08-29T17:15:16Z

starfish/core/intensity_table/decoded_intensity_table.py

+        return intensities
+
+    @classmethod
+    def assign_synthetic_targets(cls, intensities: IntensityTable) -> "DecodedIntensityTable":


Since this is used only for tests, I would consider moving this outside of the class to keep things easier for users.

ttung · 2019-08-29T17:16:00Z

starfish/core/intensity_table/decoded_intensity_table.py

+        """
+
+    @classmethod
+    def synthetic_intensities(


I think this is only for tests. If true, I would consider moving this outside of the class to keep things easier for users.

I haven't heard of our users leveraging this module, so @ttung 's suggestion to move it out seems reasonable.

ttung · 2019-08-29T17:18:23Z

starfish/core/intensity_table/decoded_intensity_table.py

+            targets=(Features.AXIS, np.random.choice(list('ABCD'), size=20)),
+            distances=(Features.AXIS, np.random.rand(20)))
+
+    def assign_cell_ids(self, cell_ids: Tuple[str, np.ndarray]):


Out of the scope of this PR, but I question whether this method makes sense in this class. Intuitively, it seems misplaced, but I am curious what you and @ambrosejcarr think.

I strongly feel, however, if we keep this here, that we should make it an internal API (prefixed with _).

Agree on internal if retained. @ttung is your intuition is that users should be leveraging AssignTargets? Makes sense.

I'll remove this method and just assign targets directly in tests

ambrosejcarr · 2019-09-13T13:07:21Z

starfish/core/intensity_table/decoded_intensity_table.py

+        """
+
+    @classmethod
+    def synthetic_intensities(


I haven't heard of our users leveraging this module, so @ttung 's suggestion to move it out seems reasonable.

ambrosejcarr · 2019-09-13T13:08:03Z

starfish/core/intensity_table/decoded_intensity_table.py

+
+        """
+
+        # TODO nsofroniew: right now there is no jitter on x-y positions of the spots


ambrosejcarr · 2019-09-13T13:14:49Z

starfish/core/intensity_table/decoded_intensity_table.py

+            targets=(Features.AXIS, np.random.choice(list('ABCD'), size=20)),
+            distances=(Features.AXIS, np.random.rand(20)))
+
+    def assign_cell_ids(self, cell_ids: Tuple[str, np.ndarray]):


Agree on internal if retained. @ttung is your intuition is that users should be leveraging AssignTargets? Makes sense.

ambrosejcarr · 2019-09-13T13:22:19Z

starfish/core/intensity_table/decoded_intensity_table.py

+        """
+        self[Features.CELL_ID] = cell_ids
+
+    def to_decoded_spots(self) -> DecodedSpots:


This name confuses me now that the class is explicitly "decoded". I suggest to_dataframe.

xarray already has a to_dataframe() method so we can't override that, how about to_decoded_dataframe?

starfish/core/intensity_table/decoded_intensity_table.py

ambrosejcarr · 2019-09-13T13:27:10Z

starfish/core/intensity_table/decoded_intensity_table.py

+from starfish.core.util.dtype import preserve_float_range
+
+
+class DecodedIntensityTable(IntensityTable):


I wonder if both IntensityTable and DecodedIntensityTable should subclass a common AbstractClass, instead of having linear inheritance. I don't think the decoding machinery is needed by DecodedIntensityTable

Alternatively, @ttung is consistent in wanting functional code like decoding to live in its own component (see his comments about gene assignment). If going this way, it would be out of scope for this PR, but should be captured in a follow-up issue.

ttung

Looks good, please see comments before landing.

ttung · 2019-09-19T18:03:58Z

starfish/core/codebook/codebook.py

+            return DecodedIntensityTable.from_intensity_table(
+                intensities,
+                targets=(Features.AXIS, np.empty(0, dtype='U')),
+                distances=(Features.AXIS, np.empty(0, dtype=float)),


np.float64 like above.

ttung · 2019-09-19T18:05:00Z

starfish/core/intensity_table/decoded_intensity_table.py

+
+class DecodedIntensityTable(IntensityTable):
+    """
+    DecodedIntensityTable is a container for spot or pixel features extracted from


unwrap as per 100-col convention, pls.

ttung · 2019-09-19T18:08:15Z

starfish/core/intensity_table/decoded_intensity_table.py

+        Generates a dataframe containing decoded spot information. Guaranteed to contain physical
+        spot coordinates (z, y, x) and gene target. Does not contain pixel coordinates.
+        """
+        if Features.TARGET not in self.coords.keys():


This is not necessary any more.

ttung · 2019-09-19T18:08:24Z

starfish/core/intensity_table/decoded_intensity_table.py

+        """
+
+        # verify the IntensityTable has been decoded
+        if Features.TARGET not in self.coords.keys():


This is also not necessary any more.

ttung · 2019-09-19T18:10:05Z

starfish/core/intensity_table/test/test_intensity_table_coords.py

@@ -7,6 +7,7 @@

 from starfish.core.codebook.test.factories import codebook_array_factory
 from starfish.core.imagestack.test.factories import imagestack_with_coords_factory
+from starfish.core.intensity_table.test import factories


This should be a relative import, i.e., from .factories import synthetic_decoded_intenisty_table

ttung · 2019-09-19T18:12:41Z

Oh, can you verify the docs generate correctly for this? I think we need to link it in from the .rst files.

shanaxel42 requested a review from ttung August 13, 2019 18:47

shanaxel42 mentioned this pull request Aug 20, 2019

Refactoring Spot Finding TAKE 2 #1500

Closed

ttung requested changes Aug 29, 2019

View reviewed changes

ambrosejcarr requested changes Sep 13, 2019

View reviewed changes

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

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

ttung approved these changes Sep 19, 2019

View reviewed changes

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

adding DecodedIntensityTable

b73ab2f

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

shanaxel42 merged commit e325fb9 into master Sep 19, 2019

shanaxel42 deleted the saxelrod-decoded-intensity-table branch September 19, 2019 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding DecodedIntensityTable class #1489

Adding DecodedIntensityTable class #1489

shanaxel42 commented Aug 13, 2019 •

edited

Loading

codecov-io commented Aug 13, 2019 •

edited

Loading

ttung left a comment

ttung Aug 29, 2019

ambrosejcarr Sep 13, 2019

ttung Aug 29, 2019

ambrosejcarr Sep 13, 2019

ttung Aug 29, 2019

ttung Aug 29, 2019

ambrosejcarr Sep 13, 2019

ttung Aug 29, 2019

ambrosejcarr Sep 13, 2019

shanaxel42 Sep 18, 2019

ambrosejcarr Sep 13, 2019

ambrosejcarr Sep 13, 2019

ambrosejcarr Sep 13, 2019

ambrosejcarr Sep 13, 2019

shanaxel42 Sep 18, 2019

ambrosejcarr Sep 13, 2019

ttung left a comment

ttung Sep 19, 2019

ttung Sep 19, 2019

ttung Sep 19, 2019

ttung Sep 19, 2019

ttung Sep 19, 2019

ttung commented Sep 19, 2019


		"""

		# TODO nsofroniew: right now there is no jitter on x-y positions of the spots

		from starfish.core.util.dtype import preserve_float_range


		class DecodedIntensityTable(IntensityTable):

Adding DecodedIntensityTable class #1489

Adding DecodedIntensityTable class #1489

Conversation

shanaxel42 commented Aug 13, 2019 • edited Loading

codecov-io commented Aug 13, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttung commented Sep 19, 2019

shanaxel42 commented Aug 13, 2019 •

edited

Loading

codecov-io commented Aug 13, 2019 •

edited

Loading