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

Graph based decoding initial commit #1482

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

gapartel
Copy link

@gapartel gapartel commented Aug 5, 2019

I integrated the graph based decoding approach in starfish as a spot detection filter. The filter first calls spots for every channel and round using one of four possible detection methods (two based on local maxima and two based on blob detection). Secondly, overlapping spots are merged across channels within each round in order to handle fluorescent bleed-trough. Next, a quality score is assigned for each detected spot (maximum intensity divided by intensity vector l2-norm). Detected spots belonging to different sequencing rounds and closer than d_th are connected in a graph, forming connected components of spot detections. Next, for each connected component, edges between not connected spots belonging to consecutive rounds are forced if they are closer than dth_max. Finally, all the edges that connect spots non belonging to consecutive rounds are removed and each connected component is solved by maximum flow minimum cost algorithm. Costs are inversely proportional to spot quality and distances.
The final intensity table is then initialized with the intensity table of the round chosen as anchor (default: first round). Intensity vectors of spots (from the other rounds) contributing to a decoded sequence are then added together (in the same xarray feature) to the intensity table and can be decoded by decode_per_round_max.

@codecov-io
Copy link

codecov-io commented Aug 5, 2019

Codecov Report

Merging #1482 into master will increase coverage by 0.38%.
The diff coverage is 94.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1482      +/-   ##
==========================================
+ Coverage   87.59%   87.97%   +0.38%     
==========================================
  Files         146      147       +1     
  Lines        5053     5371     +318     
==========================================
+ Hits         4426     4725     +299     
- Misses        627      646      +19
Impacted Files Coverage Δ
starfish/core/spots/DetectSpots/__init__.py 100% <100%> (ø) ⬆️
...ore/spots/DetectSpots/local_graph_blob_detector.py 94% <94%> (ø)

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 f73a953...6666f9e. Read the comment docs.

@ambrosejcarr
Copy link
Member

Hi Gabriele, thank you so much for this PR. We're really excited to get this integrated!

To help us review, could you help us figure out how we should test this code? Specifically:

  1. Should we run it on any particular datasets in starfish, and do you have any guidance about what the results should look like, and
  2. Would you be interested in extending our testing suite in starfish/core/spots/DetectSpots/test/test_spot_detection.py? It contains a series of tests on synthetic data where we draw spots in particular locations and verify that the spot detector recovers them. it might be a bit of work for your detector since it makes more sophisticated assumptions about where spots should lie.

Let us know if you expect to have bandwidth for this, otherwise I'm happy to help!

@gapartel
Copy link
Author

gapartel commented Aug 7, 2019

Thank you Ambrose,

  1. The code could be tested on any dataset with one-hot encoding. I have a notebook that runs the code using three of the four possible spots calling method in the class and compare the results with respect to classical BlobDetector for test ISS data. I could hand it over to you if you like.

  2. Yes, no problems. I can do something similar to /starfish/core/spots/DetectSpots/test/test_local_search_blob_detector.py. Any guidelines on what these tests should cover?

@ambrosejcarr
Copy link
Member

@gpartel your suggested approach sounds great. I think uploading the notebook would be a great idea. If you wanted, I think it would be appropriate to add it to the PR, name it a variation on "graph based decoding", and add some detail on how a user would pick the parameters or expect it to work. That could help foster interest in your method.

If you want to reference a citation so that you're clearly credited that's probably ideal -- I would put that into the docstring for the class as well. You can see example of a reference to prior work in the BlobDetector class.

Regarding the tests, the test_local_search_blob_detector.py just creates a series of synthetic examples that serve as positive and negative controls so that we know that each parameter is working as intended. For example, it has cases where search_radius can be tuned so that it captures a spot, and fails to, and tests for correct operation. These tests should be on small data and run quickly. Does that answer your question?

@berl
Copy link
Collaborator

berl commented Aug 7, 2019

Right now it looks to me like this decoder is also a spotfinder. @ambrosejcarr how will this fit into the current structure in starfish/core/spots?

@ambrosejcarr
Copy link
Member

ambrosejcarr commented Sep 13, 2019

Hey @gapartel, we're very excited about getting this merged! I'm wondering if you need any additional information from me to write up the tests.

@gapartel
Copy link
Author

Hey @gapartel, we're very excited about getting this merged! I'm wondering if you need any additional information from me to write up the tests.

Hey Ambrose, sorry for leaving this hanging! I have been quite busy after coming back from summer break, but I am planning to reserve some time next week to finalize the merge. I don't think I need more info at the moment, but in case I will come back to you soon. Thank you!

@gapartel
Copy link
Author

Hej @ambrosejcarr, I finally created the unit tests and the notebook. Sorry for the several commits but I had to fix some class dependencies from the initial commit. The build is failing because a line for the new notebook is missing in .travis.yml. Do you want me to add that?

@ambrosejcarr
Copy link
Member

ambrosejcarr commented Sep 26, 2019 via email

@shanaxel42
Copy link
Collaborator

@shanaxel42 would you be able to review this when you get to a good point in the spot finding refactor?

Hey guys! Did a brief skim, looks super cool! I actually think this will be relatively easy to fit into my refactor, @gapartel do you mind if I make a new PR with your changes + the refactored spot finding modules?

@gapartel
Copy link
Author

@shanaxel42 would you be able to review this when you get to a good point in the spot finding refactor?

Hey guys! Did a brief skim, looks super cool! I actually think this will be relatively easy to fit into my refactor, @gapartel do you mind if I make a new PR with your changes + the refactored spot finding modules?

Sure, please go ahead! Thank you!

@shanaxel42
Copy link
Collaborator

hey @gapartel! I've PR'd my branch with your modified code #1613 if you wanna take a look!

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.

5 participants