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

Allow for intensity tables with labeled axes #1181

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

ttung
Copy link
Collaborator

@ttung ttung commented Apr 14, 2019

Currently, we use the offset of r/c/z to label the axes on the intensity table. We should instead be using the axes labels from the ImageStack. This PR makes that change in three key areas:

  1. IntensityTable.empty_intensity_table now accepts the labels for the axes.
  2. IntensityTable.from_spot_data now accepts the labels for the axes and verifies that the number of labels matches the intensity data.
  3. IntensityTable.from_image_stack reads the labels for the zplane axes and assigns them correctly.

Test plan: Add a test that instantiates a labeled ImageStack and derives an IntensityTable from it. Also ran make test && make -j run-notebooks.

Depends on #1178
Fixes #1168

@ttung ttung force-pushed the tonytung-labeled-intensity-tables branch from 6c2d2d2 to 608812d Compare April 14, 2019 08:27
@codecov-io
Copy link

codecov-io commented Apr 14, 2019

Codecov Report

Merging #1181 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1181      +/-   ##
==========================================
- Coverage   88.52%   88.29%   -0.23%     
==========================================
  Files         133      134       +1     
  Lines        4976     5008      +32     
==========================================
+ Hits         4405     4422      +17     
- Misses        571      586      +15
Impacted Files Coverage Δ
starfish/spots/_detector/detect.py 98.3% <100%> (ø) ⬆️
starfish/imagestack/imagestack.py 93.88% <100%> (ø) ⬆️
starfish/intensity_table/intensity_table.py 96.42% <100%> (-0.68%) ⬇️
starfish/data/__init__.py 53.33% <0%> (ø)

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 7a2e5b4...bd6e533. Read the comment docs.

@ttung ttung force-pushed the tonytung-labeled-intensity-tables branch 3 times, most recently from e7c8dbd to a2f6818 Compare April 14, 2019 19:46
@ttung ttung force-pushed the tonytung-unique-tiles-provider branch from 3de5f0e to fe6b3bf Compare April 14, 2019 19:46
@ttung ttung force-pushed the tonytung-labeled-intensity-tables branch from a2f6818 to f449151 Compare April 14, 2019 19:46
Copy link
Member

@ambrosejcarr ambrosejcarr left a comment

Choose a reason for hiding this comment

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

Great change, thanks. See one question below.

starfish/intensity_table/intensity_table.py Outdated Show resolved Hide resolved
@ttung ttung force-pushed the tonytung-labeled-intensity-tables branch 2 times, most recently from 2c3b2f8 to cad0b89 Compare April 15, 2019 04:08
ymin = crop_y
xmin = crop_x
zmax = image_stack.shape['z'] - crop_z
zmax = image_stack.axis_labels(Axes.ZPLANE)[-crop_z - 1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

-crop_z - 1 this is weird, what value is this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this is to allow for cropping from the maximum zlayer. crop_z=0 => zmax=-1 (i.e., last plane). crop_z=1 => zmax=-2 (i.e., second to last plane).

@ttung ttung force-pushed the tonytung-unique-tiles-provider branch from 097ebde to c7328da Compare April 16, 2019 16:00
@ttung ttung force-pushed the tonytung-labeled-intensity-tables branch 5 times, most recently from a74f9ae to 4062d6f Compare April 17, 2019 07:23
@ttung ttung changed the base branch from tonytung-unique-tiles-provider to master April 17, 2019 07:26
@ttung ttung force-pushed the tonytung-labeled-intensity-tables branch 2 times, most recently from 8d3311a to 74be1d7 Compare April 17, 2019 17:29
Currently, we use the offset of r/c/z to label the axes on the intensity table.  We should instead be using the axes labels from the ImageStack.  This PR makes that change in three key areas:

1. `IntensityTable.empty_intensity_table` now accepts the labels for the axes.
2. `IntensityTable.from_spot_data` now accepts the labels for the axes and verifies that the number of labels matches the intensity data.
3. `IntensityTable.from_image_stack` reads the labels for the zplane axes and assigns them correctly.

Test plan: Add a test that instantiates a labeled ImageStack and derives an IntensityTable from it.  Also ran `make test && make -j run-notebooks`.

Depends on #1178
Fixes #1168
@ttung ttung force-pushed the tonytung-labeled-intensity-tables branch from 74be1d7 to bd6e533 Compare April 18, 2019 00:11
@ttung ttung merged commit 64f3e5a into master Apr 18, 2019
@ttung ttung deleted the tonytung-labeled-intensity-tables branch April 18, 2019 05:21
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.

Add test to verify that ImageStack with labeled axes -> IntensityTable works correctly
4 participants