-
Notifications
You must be signed in to change notification settings - Fork 68
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
[RFC] Add linear unmixing #987
Conversation
Codecov Report
@@ Coverage Diff @@
## master #987 +/- ##
==========================================
+ Coverage 88.59% 89.11% +0.51%
==========================================
Files 162 168 +6
Lines 5824 6514 +690
==========================================
+ Hits 5160 5805 +645
- Misses 664 709 +45
Continue to review full report at Codecov.
|
In terms of checking the functionality, I think the following test could be enough to get us started. I haven't used pytest. Is using an Also, would this go in import numpy as np
from starfish.imagestack.imagestack import ImageStack
import starfish
# Create image
im = np.ones((2, 3, 5, 2, 2))
stack = ImageStack.from_numpy_array(im)
# Create reference result
ref_result = np.ones((2, 3, 5, 2, 2))
ref_result[:, 1, ...] = 0.5 * np.ones((5, 2, 2))
# Unmix
coeff_mat = np.array([[1, 0, 0], [-0.25, 1, -0.25], [0, 0, 1]])
filter_unmix = starfish.image.Filter.LinearUnmixing(coeff_mat=coeff_mat)
stack2 = filter_unmix.run(stack, in_place=False, verbose=False)
# Check result
assert np.all(ref_result == stack2.xarray.values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I've left some comments, and responses to your questions:
- I think it would be nice to expose a couple of rescaling options. Perhaps normalizing to the max and/or mean?
We have some separate normalization components (scale by percentile, I'm going to add histogram matching soon). Mean could work. Max seems like it would fit to outliers? I think I'd keep these separate because there are good reasons for these to be more general than per-channel (see below).
- This assumes the unmixing will be performed across channels. Is there value in making it general? At the moment, I can't think of any use cases, so I'm leaning towards keeping it specific to channels.
I couldn't think of anything either. Keeping it channel specific makes sense to me.
Also, would this go in
starfish/test/pipeline
?
I'd put it in starfish/test/image/filter
|
||
Parameters | ||
---------- | ||
image : np.ndarray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be great if it took an xarray
with labeled axes. Then later you could make this change:
- n_channels = image.shape[0]
- for c in range(n_channels):
+ for c in image.coords[Axes.CH]:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would be nice to use the axis labels for safety, but the apply()
passes a numpy array. I propose that we leave it as is for now and make and issue proposing that we change the apply()
method to pass an xarray. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #1035
# Due to the grouping, assumes image is shape [chan, x, y] | ||
n_channels = image.shape[0] | ||
|
||
for c in range(n_channels): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The framing of B = AX
makes me think there's a one-liner that captures the logic in this for loop. Does that seem crazy? I'd be happy to talk through this on a call. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intuition was the same. However, I couldn't figure out how to do it without the for loop. Within the for loop (i.e., for a given channel), we could do something like:
np.dot(np.transpose(image[c, :, :]), coeff_mat[c])
but originally, I thought my previous implementation was more clear. I'm definitely not a broadcasting wizard, so if you can figure out how to reshape the matrices to do it all in one line, I'd love to learn.
|
Co-Authored-By: kevinyamauchi <kevin.yamauchi@gmail.com>
@ambrosejcarr I've added a test and addressed your comments. Would you mind taking a look? |
Closed by #1045 |
Objective
This PR introduces a component to perform linear unmixing across channels to compensate for crosstalk between filters.
Overview
Per our discussion in #945, I implemented the following:
Usage
Questions