3.) SeqFish/StarMap/LocalBlobSearch #1592

shanaxel42 · 2019-10-01T14:18:17Z

Both STARmap and current Seqfish currently use LocalSearchBlobDetector. This refactor eliminates the need for a separate LocalSearchBlobDetector class and instead both assays now use the regular BlobDetector and the PerRoundMaxChannel with a trace building strategy of 'nearest neighbors'. A lot of the changes here are mostly just moving code that already existed in LocalSearchBlobDetector and a few changes to make BlobDetector work for both ISS and STARmap.

depends on: #1518 #1517

codecov-io · 2019-10-01T14:29:37Z

Codecov Report

Merging #1592 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1592      +/-   ##
==========================================
+ Coverage   88.04%   88.26%   +0.21%     
==========================================
  Files         151      152       +1     
  Lines        5171     5241      +70     
==========================================
+ Hits         4553     4626      +73     
+ Misses        618      615       -3

Impacted Files	Coverage Δ
starfish/types.py	`100% <ø> (ø)`	⬆️
starfish/core/types/__init__.py	`100% <ø> (ø)`	⬆️
starfish/core/spots/DecodeSpots/trace_builders.py	`100% <100%> (ø)`	⬆️
starfish/core/types/_constants.py	`98.21% <100%> (+0.1%)`	⬆️
...re/spots/DetectSpots/local_search_blob_detector.py	`95.23% <100%> (+0.04%)`	⬆️
starfish/core/types/_spot_finding_results.py	`96.96% <100%> (+0.19%)`	⬆️
...spots/DecodeSpots/per_round_max_channel_decoder.py	`100% <100%> (ø)`	⬆️
starfish/core/spots/DecodeSpots/util.py	`100% <100%> (ø)`
starfish/core/spots/FindSpots/blob.py	`95.91% <100%> (+6.78%)`	⬆️
... and 1 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 58eb7ac...cd62776. Read the comment docs.

ttung · 2019-10-07T18:30:02Z

notebooks/py/STARmap.py

+                                           threshold=np.percentile(np.ravel(stack.xarray.values), 95),
+                                           exclude_border=2)
+
+spots = bd.run(scaled, n_processes=8)


Is there a reason why this has an explicit n_processes setting?

I have no idea, changed it to the default

It's also possible that it was done to survive travis...?

ttung · 2019-10-07T19:04:26Z

notebooks/py/STARmap.py

+decoder = starfish.spots.DecodeSpots.PerRoundMaxChannel(codebook=experiment.codebook,
+                                                        anchor_round=0,
+                                                        search_radius=10,
+                                                        trace_building_strategy=
+                                                        TraceBuildingStrategies.NEAREST_NEIGHBOR)


Generally, if you stick the parameters onto the next line, it'll format better.

Suggested change

decoder = starfish.spots.DecodeSpots.PerRoundMaxChannel(codebook=experiment.codebook,

anchor_round=0,

search_radius=10,

trace_building_strategy=

TraceBuildingStrategies.NEAREST_NEIGHBOR)

decoder = starfish.spots.DecodeSpots.PerRoundMaxChannel(

codebook=experiment.codebook,

anchor_round=0,

search_radius=10,

trace_building_strategy=TraceBuildingStrategies.NEAREST_NEIGHBOR)

ttung · 2019-10-07T20:41:11Z

notebooks/py/SeqFISH.py

+                                           threshold=threshold)
+
+spots = bd.run(clipped)
+decoder = starfish.spots.DecodeSpots.PerRoundMaxChannel(codebook=exp.codebook,


same as above.

ttung · 2019-10-07T20:44:01Z

starfish/core/spots/DecodeSpots/per_round_max_channel_decoder.py

+    def __init__(
+            self,
+            codebook: Codebook,
+            anchor_round: Optional[int]=1,


I think the parameter order you have in the docblock makes more sense as the optional params only pertain to certain selections of trace_building_strategy.

ttung · 2019-10-07T20:45:16Z

starfish/core/spots/DecodeSpots/trace_builders.py

+def build_traces_nearest_neighbors(spot_results: SpotFindingResults, anchor_round: int=1,
+                                   search_radius: int=3):
+    """
+    Combine spots found across round and channels of ana ImageStack using a nearest neighbors


Suggested change

Combine spots found across round and channels of ana ImageStack using a nearest neighbors

Combine spots found across round and channels of an ImageStack using a nearest neighbors

ttung · 2019-10-08T00:30:14Z

starfish/core/spots/DecodeSpots/util.py

+        feature_index = np.arange(ind.shape[0], dtype=int)
+
+        # mask spots that are outside the search radius
+        mask = np.asarray(dist[r] < search_radius)  # indices need not match


meta question (and out of the scope of this PR), but why is distance in x-y space treated the same as distance in z-space? intuitively that seems incorrect to me.

that's a good question @ambrosejcarr ?

Given the properties of the data acquisition, In most cases it should not be treated the same (although there are exceptions). This is the way the search was implemented by the authors of the pipeline, and so it is how we implemented it. If it is simple to set search radii differently for x, y, z, that would be an improvement.

that's a really good question

I propose that we file an issue to discuss this, and perhaps engage some ppl who use this approach to discuss. It may not matter in the end, but it does not currently pass the smell test. :)

ttung · 2019-10-08T00:33:26Z

starfish/core/spots/DecodeSpots/util.py

+) -> Dict[int, pd.DataFrame]:
+    """Merge DataFrames containing spots from different channels into one DataFrame per round.
+
+    Executed on the output of


ttung · 2019-10-08T21:34:54Z

notebooks/py/STARmap.py

+decoder = starfish.spots.DecodeSpots.PerRoundMaxChannel(codebook=experiment.codebook,
+                                                        anchor_round=0,
+                                                        search_radius=10,
+                                                        trace_building_strategy=
+                                                        TraceBuildingStrategies.NEAREST_NEIGHBOR)
+
+decoded = decoder.run(spots=spots)


This should go after the next markdown block.

ttung · 2019-10-08T21:41:20Z

starfish/core/spots/DecodeSpots/test/test_nearest_neighbor_trace_builder.py

+from starfish.core.types import Axes
+
+
+def traversing_code() -> ImageStack:


to be really exhaustive, there should probably be a test case for testing the distance filter.

which distance filter? the search_radius param?

Basically to make sure that trace building deals with the search_radius param correctly.

ahh got it. More then what's already in the jitter_code test where it checks that it doesn't build traces when the search radius is below the 3px distance?

ttung · 2019-10-08T21:42:53Z

starfish/core/spots/DecodeSpots/util.py

+    for r in sorted(set(round_dataframes.keys()) - {anchor_round, }):
+        query_df = round_dataframes[r]
+        query_coordinates = query_df[[Axes.ZPLANE, Axes.Y, Axes.X]]
+        nn = NearestNeighbors(n_neighbors=1)


does NN not support a distance filter intrinsically?

it looks like no there's a metric param that might work. But probs another question for @ambrosejcarr

ttung · 2019-10-10T06:37:01Z

starfish/core/spots/DecodeSpots/test/test_nearest_neighbor_trace_builder.py

+    assert np.all(c == np.array([0, 0, 1]))
+    assert np.all(r == np.array([0, 2, 1]))
+
+    # test again with smaller search radius


ttung · 2019-10-10T06:40:07Z

starfish/core/spots/DecodeSpots/util.py

+        feature_index = np.arange(ind.shape[0], dtype=int)
+
+        # mask spots that are outside the search radius
+        mask = np.asarray(dist[r] < search_radius)  # indices need not match


I propose that we file an issue to discuss this, and perhaps engage some ppl who use this approach to discuss. It may not matter in the end, but it does not currently pass the smell test. :)

ttung · 2019-10-10T06:40:53Z

Tests are failing though.

shanaxel42 changed the title ~~SeqFish/StarMap/LocalSearch~~ 3.) SeqFish/StarMap/LocalSearch Oct 1, 2019

shanaxel42 changed the title ~~3.) SeqFish/StarMap/LocalSearch~~ SeqFish/StarMap/LocalBlobSearch Oct 1, 2019

shanaxel42 changed the title ~~SeqFish/StarMap/LocalBlobSearch~~ 3.) SeqFish/StarMap/LocalBlobSearch Oct 1, 2019

shanaxel42 requested review from ttung and ambrosejcarr October 1, 2019 15:05

shanaxel42 force-pushed the saxelrod-starMap-refactor branch 2 times, most recently from cffb21c to 5996a09 Compare October 1, 2019 15:32

This was referenced Oct 1, 2019

4.) refactoring allen smFish with new spot finding #1593

Merged

5.) refactoring localMaxPeakFinder/osmFIsh #1594

Merged

6.)adding MetricDecoder to DecodeSpots #1596

Merged

shanaxel42 force-pushed the saxelrod-starMap-refactor branch from 5996a09 to 8dd6a0b Compare October 3, 2019 17:42

shanaxel42 mentioned this pull request Oct 7, 2019

2) ISS spot finding refactor #1518

Merged

shanaxel42 force-pushed the saxelrod-starMap-refactor branch 4 times, most recently from ed13a8b to 6d4a553 Compare October 8, 2019 17:31

ttung requested changes Oct 8, 2019

View reviewed changes

shanaxel42 force-pushed the saxelrod-starMap-refactor branch from 6d4a553 to f4c676c Compare October 8, 2019 22:48

shanaxel42 force-pushed the saxelrod-iss-new-spot-finding branch 2 times, most recently from e6c6431 to cf170a5 Compare October 8, 2019 22:56

shanaxel42 force-pushed the saxelrod-starMap-refactor branch 2 times, most recently from 147cd1a to c80190f Compare October 9, 2019 15:24

shanaxel42 changed the base branch from saxelrod-iss-new-spot-finding to master October 9, 2019 15:25

shanaxel42 force-pushed the saxelrod-starMap-refactor branch 4 times, most recently from 1f4858f to df82f1e Compare October 9, 2019 21:55

shanaxel42 requested a review from ttung October 9, 2019 21:56

shanaxel42 force-pushed the saxelrod-starMap-refactor branch from df82f1e to a900ebd Compare October 9, 2019 23:25

ttung reviewed Oct 10, 2019

View reviewed changes

ttung approved these changes Oct 10, 2019

View reviewed changes

implementing starMap refactor

cd62776

shanaxel42 force-pushed the saxelrod-starMap-refactor branch from a900ebd to cd62776 Compare October 10, 2019 15:15

shanaxel42 merged commit 6bf1d96 into master Oct 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3.) SeqFish/StarMap/LocalBlobSearch #1592

3.) SeqFish/StarMap/LocalBlobSearch #1592

shanaxel42 commented Oct 1, 2019 •

edited

Loading

codecov-io commented Oct 1, 2019 •

edited

Loading

ttung Oct 7, 2019

shanaxel42 Oct 9, 2019

ttung Oct 9, 2019

ttung Oct 7, 2019

ttung Oct 7, 2019

ttung Oct 7, 2019

shanaxel42 Oct 9, 2019

ttung Oct 7, 2019

ttung Oct 8, 2019

shanaxel42 Oct 9, 2019

ambrosejcarr Oct 9, 2019

dganguli Oct 9, 2019

ttung Oct 10, 2019

ttung Oct 8, 2019

ttung Oct 8, 2019

ttung Oct 8, 2019

shanaxel42 Oct 9, 2019

ttung Oct 9, 2019

shanaxel42 Oct 9, 2019

ttung Oct 8, 2019

shanaxel42 Oct 9, 2019

ttung Oct 10, 2019

ttung Oct 10, 2019

ttung commented Oct 10, 2019

	Combine spots found across round and channels of ana ImageStack using a nearest neighbors
	Combine spots found across round and channels of an ImageStack using a nearest neighbors

		from starfish.core.types import Axes


		def traversing_code() -> ImageStack:

3.) SeqFish/StarMap/LocalBlobSearch #1592

3.) SeqFish/StarMap/LocalBlobSearch #1592

Conversation

shanaxel42 commented Oct 1, 2019 • edited Loading

codecov-io commented Oct 1, 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

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 commented Oct 10, 2019

shanaxel42 commented Oct 1, 2019 •

edited

Loading

codecov-io commented Oct 1, 2019 •

edited

Loading