Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Generalized masking #1092
Generalized masking #1092
Changes from 14 commits
197cd84
9f4325c
2575a21
f288cb6
348caf5
8b8566d
3a8a908
15ae88a
02032ed
3771b07
a9ecb44
f55911e
4b1f9c8
5b024c6
a1fc940
c5b0e67
04b8412
1572f9e
34a32e8
3c48aa4
61e0b85
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why do we not want to return the composite images here as well? For example what if the user passes in a path for
log_dir
but there isn't a path for thecomposite_directory
. It wouldn't return the dictionary nor save any of the composite files.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 ezseg functions only save the composites, they don't return them so I would have to change the ezseg notebook to handle a return (not a big deal, but unnecessary when it's not even used). Since the only case we would need the composites returned is for the internal function call inside generate_signal_masks(), I didn't think it was needed.
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.
In what instance would this not be a numpy array? If it's an xarray, all normal numpy ops and functions which make use of numpy ops will work, so we wouldn't need to convert it.
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.
Where I'm implementing it in masking_utils,
img2mask
is already an np.array, so I get an AttributeError: 'numpy.ndarray' object has no attribute 'to_numpy'.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 would add a few defaults here:
cell_types
: default to None, in which case include all cell types increate_cell_masks
cluster_col
: should be ark.settings.cell_type. In that way you may not have to explicitly specify it in the notebook.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
cluster_col
default is a great idea! Forcell_types
, it wouldn't really make sense to mask all cell types in the image (it would just be a blurred version of the segmentation masks), so forcing the user to specify a list is robably the way to go.