replace label images with segmentation masks #1135

kne42 · 2019-04-04T20:42:07Z

closes #875

ttung

comment checkpoint.

starfish/segmentation_mask.py

ttung · 2019-04-05T22:49:29Z

starfish/segmentation_mask.py

+
+    Parameters
+    ----------
+    masks : list of xr.DataArray


What do you foresee as the access pattern for these masks?

The whole collection will effectively serve as a proxy to these. Do you think it's better practice to expose this as a public attribute?

What I would normally/prefer to do is not create a class at all and just use a functional approach which is much cleaner IMO (outside of type hints) but I wanted this to blend in with the rest of the package which feels a bit class-happy tbh.

starfish/segmentation_mask.py

ttung · 2019-04-05T23:18:15Z

starfish/segmentation_mask.py

+        ----------
+        path : str
+            Path of the directory to write to.
+        overwrite : bool, optional


I think the pattern used thus far in this repo is to do:

overwrite : bool Whether to overwrite the directory if it exists. (default: False)

If you don't feel strongly about it, let's stick with this convention. If you do feel strongly about it, let's discuss. :)

I don't feel too strongly about this although I do think that the , optional part is useful to users and the (default: False) is not so much since they can just take a look at the signature for the default value.

starfish/segmentation_mask.py

ambrosejcarr

Looks like there is some issue from the CLI call with the validation command (see travis). Otherwise this looks quite good! I've made a bunch of minor comments.

starfish/segmentation_mask.py

ambrosejcarr · 2019-04-07T23:54:41Z

starfish/spots/_target_assignment/label.py

        intensities: IntensityTable,
        in_place: bool,
    ) -> IntensityTable:
+        cell_ids = []
+
+        for spot in intensities:


I think this is worth a short comment on the strategy. "for each spot, test whether the spot falls inside the area of each mask"

Second: I'm guessing this is really fast because there just aren't that many spot/mask combinations? If it's not, we could build an interval tree over the mask dimensions to speed this up.

For our test cases there aren't many, sure. If you think that there are users who (will) have a lot of spots/masks then we could look into improving the algorithm.

starfish/image/_segmentation/watershed.py

shanaxel42

Good work! I agree with the comments Ambrose and Tony made about adding some comments and making sure not to have any of the code dependent on axis ordering, but overall awesome!

starfish/image/_segmentation/_base.py

starfish/test/test_segmentation_mask.py

codecov-io · 2019-04-10T20:22:20Z

Codecov Report

Merging #1135 into master will decrease coverage by 2.21%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #1135      +/-   ##
==========================================
- Coverage   90.47%   88.25%   -2.22%     
==========================================
  Files         125      129       +4     
  Lines        4566     4931     +365     
==========================================
+ Hits         4131     4352     +221     
- Misses        435      579     +144

Impacted Files	Coverage Δ
starfish/test/full_pipelines/cli/test_iss.py	`100% <ø> (ø)`	⬆️
starfish/image/_segmentation/watershed.py	`96.72% <100%> (+0.05%)`	⬆️
starfish/image/_segmentation/_base.py	`97.14% <100%> (+0.08%)`	⬆️
starfish/spots/_target_assignment/_base.py	`97.22% <100%> (ø)`	⬆️
starfish/test/full_pipelines/api/test_iss_api.py	`100% <100%> (ø)`	⬆️
starfish/segmentation_mask.py	`93.58% <93.58%> (ø)`
starfish/spots/_target_assignment/label.py	`94.44% <93.75%> (+5.55%)`	⬆️
starfish/util/argparse.py	`0% <0%> (-85.72%)`	⬇️
starfish/compat.py	`10.84% <0%> (-55.83%)`	⬇️
starfish/imagestack/parser/tileset/_parser.py	`81.69% <0%> (-3.26%)`	⬇️
... and 51 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 d2bc0f0...c8df372. Read the comment docs.

kne42 · 2019-04-11T20:36:30Z

Ok, thanks for all the comments! I've addressed them (I hope) so this should be ready for re-review :)

kne42 · 2019-04-11T20:40:59Z

There are also some nice features I could add such as enforcing unique label names which would open up the possibility to index by label or lazy load from disk.

ttung · 2019-04-12T17:50:51Z

Ok, thanks for all the comments! I've addressed them (I hope) so this should be ready for re-review :)

when you want to ask for a re-review, you can do so using the github UI. This way, it shows up on the reviewers' dashboards.

ttung

There's a couple small lint issues that's preventing this from going green. Generally this looks great now!

ttung · 2019-04-12T19:17:39Z

starfish/segmentation_mask.py

+    masks : list of xr.DataArray
+        Segmentation masks.
+    """
+    _masks: List[xr.DataArray]


I think the constructor assigning this variable implicitly types the field.

starfish/segmentation_mask.py

ambrosejcarr

👍

replace label images with segmentation masks

856314c

kne42 requested review from ambrosejcarr, ttung and shanaxel42 April 4, 2019 20:42

ttung reviewed Apr 5, 2019

View reviewed changes

ambrosejcarr mentioned this pull request Apr 6, 2019

Fix Target Assignment for CLI #1088

Closed

ambrosejcarr reviewed Apr 8, 2019

View reviewed changes

shanaxel42 reviewed Apr 8, 2019

View reviewed changes

starfish/image/_segmentation/_base.py Outdated Show resolved Hide resolved

starfish/test/test_segmentation_mask.py Show resolved Hide resolved

kne42 added 2 commits April 9, 2019 11:18

improve docs & organization

460f691

save as tar file instead of directory

e21d700

kne42 added 2 commits April 10, 2019 17:10

update cli

06a3e24

more concise validation using sets

bfd6ec0

ttung approved these changes Apr 12, 2019

View reviewed changes

various syntax/organization changes

c8df372

ambrosejcarr approved these changes Apr 15, 2019

View reviewed changes

kne42 merged commit 9d4bf1d into spacetx:master Apr 15, 2019

kne42 deleted the segmentation-mask branch April 15, 2019 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace label images with segmentation masks #1135

replace label images with segmentation masks #1135

kne42 commented Apr 4, 2019 •

edited

Loading

ttung left a comment

ttung Apr 5, 2019

kne42 Apr 11, 2019

ttung Apr 5, 2019

kne42 Apr 11, 2019

ambrosejcarr left a comment

ambrosejcarr Apr 7, 2019

kne42 Apr 11, 2019

shanaxel42 left a comment

codecov-io commented Apr 10, 2019 •

edited

Loading

kne42 commented Apr 11, 2019

kne42 commented Apr 11, 2019

ttung commented Apr 12, 2019

ttung left a comment

ttung Apr 12, 2019

ambrosejcarr left a comment

replace label images with segmentation masks #1135

replace label images with segmentation masks #1135

Conversation

kne42 commented Apr 4, 2019 • edited Loading

ttung left a comment

Choose a reason for hiding this comment

ttung Apr 5, 2019

Choose a reason for hiding this comment

kne42 Apr 11, 2019

Choose a reason for hiding this comment

ttung Apr 5, 2019

Choose a reason for hiding this comment

kne42 Apr 11, 2019

Choose a reason for hiding this comment

ambrosejcarr left a comment

Choose a reason for hiding this comment

ambrosejcarr Apr 7, 2019

Choose a reason for hiding this comment

kne42 Apr 11, 2019

Choose a reason for hiding this comment

shanaxel42 left a comment

Choose a reason for hiding this comment

codecov-io commented Apr 10, 2019 • edited Loading

Codecov Report

kne42 commented Apr 11, 2019

kne42 commented Apr 11, 2019

ttung commented Apr 12, 2019

ttung left a comment

Choose a reason for hiding this comment

ttung Apr 12, 2019

Choose a reason for hiding this comment

ambrosejcarr left a comment

Choose a reason for hiding this comment

kne42 commented Apr 4, 2019 •

edited

Loading

codecov-io commented Apr 10, 2019 •

edited

Loading