Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Oct 15, 2019
Merged

Conversation

shanaxel42
Copy link
Collaborator

@shanaxel42 shanaxel42 commented Oct 1, 2019

This PR:

  • adds a version of TrackpyLocalMaxPeakFinder to the new FindSpots module that process SpotFindingResults
  • Adds a new trace building strategy to use in decoding called SEQUENTIAL that builds spot traces without merging across channels and imaging rounds. Used for sequential methods like smFIsh.
  • refactors smFish to use FindSpots.TrackpyLocalMaxPeakFinder DecodeSpots.PerRoundMaxChannel with the trace building strategy SEQUENTIAL

depends on: #1592 #1518 #1517

@codecov-io
Copy link

codecov-io commented Oct 1, 2019

Codecov Report

Merging #1593 into master will decrease coverage by 1.96%.
The diff coverage is 28.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1593      +/-   ##
==========================================
- Coverage   88.18%   86.22%   -1.97%     
==========================================
  Files         152      155       +3     
  Lines        5241     5516     +275     
==========================================
+ Hits         4622     4756     +134     
- Misses        619      760     +141
Impacted Files Coverage Δ
...spots/DetectSpots/trackpy_local_max_peak_finder.py 90.69% <0%> (-2.16%) ⬇️
starfish/data.py 50% <0%> (-3.34%) ⬇️
starfish/core/spots/FindSpots/__init__.py 100% <100%> (ø) ⬆️
starfish/core/types/_constants.py 98.24% <100%> (+0.03%) ⬆️
starfish/core/spots/DecodeSpots/__init__.py 100% <100%> (ø) ⬆️
...fish/core/spots/FindSpots/local_max_peak_finder.py 22.54% <22.54%> (ø)
starfish/core/spots/DecodeSpots/trace_builders.py 68.96% <25%> (-31.04%) ⬇️
...e/spots/FindSpots/trackpy_local_max_peak_finder.py 30% <30%> (ø)
...rfish/test/full_pipelines/api/test_allen_smFISH.py 33.33% <37.5%> (ø) ⬆️
starfish/core/spots/DecodeSpots/metric_decoder.py 45% <45%> (ø)
... and 7 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 88c5ad2...1623205. Read the comment docs.

@shanaxel42 shanaxel42 requested a review from ttung October 1, 2019 15:47
@shanaxel42 shanaxel42 force-pushed the fix-branch branch 2 times, most recently from b79759b to d971c14 Compare October 8, 2019 17:31
@@ -123,6 +125,11 @@ def processing_pipeline(
print("Loading images...")
images = enumerate(experiment[fov_name].get_images(FieldOfView.PRIMARY_IMAGES))

decoder = starfish.spots.DecodeSpots.PerRoundMaxChannel(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

following the other components of this pipeline, this should be above (search for "define")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to put it above but there's no access to the experiment.codebook at that point

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I see.

spot_attributes = tlmpf.run(primary_image)
all_intensities.append(spot_attributes)
spots = tlmpf.run(primary_image)
decoded_intensities = decoder.run(spots)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add print("Decoding spots...")

@@ -66,5 +103,6 @@ def build_traces_nearest_neighbors(spot_results: SpotFindingResults, anchor_roun

trace_builders: Mapping[TraceBuildingStrategies, Callable] = {
TraceBuildingStrategies.EXACT_MATCH: build_spot_traces_exact_match,
TraceBuildingStrategies.NEAREST_NEIGHBOR: build_traces_nearest_neighbors
TraceBuildingStrategies.NEAREST_NEIGHBOR: build_traces_nearest_neighbors,
TraceBuildingStrategies.SEQUENTIAL: build_traces_sequential
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TraceBuildingStrategies.SEQUENTIAL: build_traces_sequential
TraceBuildingStrategies.SEQUENTIAL: build_traces_sequential,

Parameters
----------

spot_diameter : odd integer or tuple of odd integers.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. params are not actually typed to match what is in the description.
  2. the params' types in the documentation does not follow the standard style we use (which is whatever is the typing in the actual function declaration).
  3. the styling for defaults does not match the styling for defaults in earlier PRs.
  4. some defaults are missing.



@pytest.mark.skip('issues with checksums prevent this data from working properly')
@pytest.mark.skip('This test runs but takes forever')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahahahaha

For what it's worth, there are a few allen smfish FOVs that have relatively few spots and run relatively quickly (2-3 minutes?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know which ones?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fov_004 has very few spots.

@@ -123,6 +125,11 @@ def processing_pipeline(
print("Loading images...")
images = enumerate(experiment[fov_name].get_images(FieldOfView.PRIMARY_IMAGES))

decoder = starfish.spots.DecodeSpots.PerRoundMaxChannel(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I see.



@pytest.mark.skip('issues with checksums prevent this data from working properly')
@pytest.mark.skip('This test runs but takes forever')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fov_004 has very few spots.

* refactoring localMaxPeakFinder

* adding MetricDecoder to DecodeSpots (#1596)
@shanaxel42 shanaxel42 merged commit 70f53cc into master Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants