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.

Sign up for GitHub

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

Jump to bottom

5.) refactoring localMaxPeakFinder/osmFIsh #1594

Merged
merged 2 commits into from
Oct 15, 2019
Merged

5.) refactoring localMaxPeakFinder/osmFIsh #1594

merged 2 commits into from
Oct 15, 2019

Conversation

shanaxel42
Copy link
Collaborator

@shanaxel42 shanaxel42 commented Oct 1, 2019 •
edited
Loading

This PR:

  • adds a new version of LocalMaxPeakFinder to the FindSpots module that processes SpotFindingResults
  • Refactors osmFish to use FindSpots.LocalMaxPeakFinder and DecodeSpots.PerRoundMaxChannel with a trace building strategy of SEQUENTIAL

depends on: #1593 #1592 #1518 #1517

@codecov-io
Copy link

codecov-io commented Oct 1, 2019 •
edited
Loading

Codecov Report

Merging #1594 into fix-branch will decrease coverage by 1.45%.
The diff coverage is 27.41%.

Impacted file tree graph

@@              Coverage Diff               @@
##           fix-branch    #1594      +/-   ##
==============================================
- Coverage       87.57%   86.11%   -1.46%     
==============================================
  Files             153      155       +2     
  Lines            5392     5438      +46     
==============================================
- Hits             4722     4683      -39     
- Misses            670      755      +85
Impacted Files Coverage Δ
starfish/core/spots/FindSpots/__init__.py 100% <100%> (ø) ⬆️
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/metric_decoder.py 45% <45%> (ø)
...arfish/core/segmentation_mask/segmentation_mask.py 94.56% <0%> (-1.82%) ⬇️
starfish/core/spots/DetectSpots/detect.py 98.36% <0%> (-0.15%) ⬇️
starfish/core/errors.py 100% <0%> (ø) ⬆️
starfish/core/imagestack/imagestack.py 93.69% <0%> (+0.68%) ⬆️
... 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 e116567...83059fc. Read the comment docs.

@shanaxel42 shanaxel42 requested review from ambrosejcarr and ttung and removed request for ttung and ambrosejcarr October 1, 2019 15:04
@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch 2 times, most recently from be8e35a to b89c389 Compare October 1, 2019 15:32
@shanaxel42 shanaxel42 requested a review from ttung October 1, 2019 15:52
@shanaxel42 shanaxel42 mentioned this pull request Oct 1, 2019
Merged
@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch 2 times, most recently from 3d96b11 to b0ca097 Compare October 7, 2019 16:18
@shanaxel42 shanaxel42 force-pushed the fix-branch branch 2 times, most recently from 82c7dc2 to 9c710e5 Compare October 7, 2019 16:33
@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch from b0ca097 to 27698d6 Compare October 7, 2019 16:33
@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch 3 times, most recently from fac9141 to f46d2ca Compare October 8, 2019 22:48
@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch from f46d2ca to 64ea392 Compare October 8, 2019 22:56
ttung
ttung approved these changes Oct 8, 2019
View reviewed changes
starfish/core/spots/FindSpots/local_max_peak_finder.py Outdated
Comment on lines 36 to 44
default 'max' calculates the maximum intensity inside the object
min_num_spots_detected : int
When fewer than this number of spots are detected, spot searching for higher threshold
values. (default = 3)
is_volume : bool
Not supported. For 3d peak detection please use TrackpyLocalMaxPeakFinder.
(default=False)
verbose : bool
If True, report the percentage completed (default = False) during processing
Copy link
Collaborator

Choose a reason for hiding this comment

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

inconsistent depiction of defaults.

starfish/core/spots/FindSpots/local_max_peak_finder.py
# stop spot finding when the number of detected spots falls below min_num_spots_detected
if len(spots) <= self.min_num_spots_detected:
stop_threshold = threshold
if self.verbose:
Copy link
Collaborator

Choose a reason for hiding this comment

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

possibly out of the scope of this PR, but this test of verbosity differs from the above one.

starfish/core/spots/FindSpots/local_max_peak_finder.py Outdated

Returns
-------
Number : #TODO ambrosejcarr this should probably be a float
Copy link
Collaborator

Choose a reason for hiding this comment

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

so, which is it, @ambrosejcarr?

Copy link
Member

Choose a reason for hiding this comment

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

float.

@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch 3 times, most recently from 9c44352 to 879f771 Compare October 9, 2019 17:50
@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch 2 times, most recently from 5ff0e1c to facfd14 Compare October 9, 2019 18:24
@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch 2 times, most recently from 1d254d9 to ff7d5f7 Compare October 9, 2019 21:55
@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch from ff7d5f7 to ba6d3be Compare October 9, 2019 22:03
@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch 3 times, most recently from 402245d to b4eb1b5 Compare October 10, 2019 19:42
@shanaxel42 shanaxel42 mentioned this pull request Oct 10, 2019
Closed
10 tasks
@shanaxel42 shanaxel42 force-pushed the saxelrod-osm-fish-new branch from b4eb1b5 to c25bbee Compare October 11, 2019 16:55
@shanaxel42 shanaxel42 merged commit 1623205 into fix-branch Oct 15, 2019
shanaxel42 pushed a commit that referenced this pull request Oct 15, 2019
4.) refactoring allen smFish with new spot finding (#1593)
70f53cc 
* refactoring allen smFish with new spot finding

* 5.) refactoring localMaxPeakFinder/osmFIsh (#1594)

* refactoring localMaxPeakFinder

* adding MetricDecoder to DecodeSpots (#1596)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambrosejcarr ambrosejcarr ambrosejcarr left review comments

@ttung ttung ttung approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shanaxel42 @codecov-io @ttung @ambrosejcarr