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

Fix on-demand calculation of BinaryMaskCollection's regionprops #1819

Merged
merged 1 commit into from
Feb 26, 2020

Conversation

ttung
Copy link
Collaborator

@ttung ttung commented Feb 25, 2020

Through most ways of constructing BinaryMaskCollections, we calculate region properties and store them with our BinaryMaskCollection. However, on a few code paths (notably from_binary_arrays_and_ticks and from_fiji_roi_set), we calculate the regionprops on demand.

This code path was not properly tested and we were not generating the regionprops correctly. This fixes the problem and adds a quick sanity test.

Test plan: test passes with changes, does not otherwise.

Through most ways of constructing BinaryMaskCollections, we calculate region properties and store them with our BinaryMaskCollection.  However, on a few code paths (notably `from_binary_arrays_and_ticks` and `from_fiji_roi_set`), we calculate the regionprops on demand.

This code path was not properly tested and we were not generating the regionprops correctly.  This fixes the problem and adds a quick sanity test.

Test plan: test passes with changes, does not otherwise.
@ttung ttung requested review from shanaxel42 and mattcai February 25, 2020 21:48
@codecov-io
Copy link

codecov-io commented Feb 25, 2020

Codecov Report

Merging #1819 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1819      +/-   ##
==========================================
- Coverage   90.11%   90.08%   -0.04%     
==========================================
  Files         254      254              
  Lines        9451     9461      +10     
==========================================
+ Hits         8517     8523       +6     
- Misses        934      938       +4
Impacted Files Coverage Δ
...ary_mask/test/test_from_binary_arrays_and_ticks.py 100% <100%> (ø) ⬆️
...tarfish/core/morphology/binary_mask/binary_mask.py 95.15% <100%> (+1.37%) ⬆️
...tarfish/core/test/test_multiprocessing_workflow.py 94.87% <0%> (-5.13%) ⬇️
starfish/core/_version.py 45.32% <0%> (-1.8%) ⬇️

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 5508316...54ea38d. Read the comment docs.

@ttung ttung merged commit ed770c5 into master Feb 26, 2020
@ttung ttung deleted the tonytung-regionprops branch February 26, 2020 14:25
@ttung ttung restored the tonytung-regionprops branch February 26, 2020 21:41
@ttung ttung deleted the tonytung-regionprops branch March 13, 2020 18:00
@ttung ttung restored the tonytung-regionprops branch March 13, 2020 18:24
@ttung ttung deleted the tonytung-regionprops branch March 13, 2020 18:25
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