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

Remove batching functionality from cell and pixel mask generation #727

Merged
merged 17 commits into from
Sep 27, 2022

Conversation

alex-l-kong
Copy link
Contributor

What is the purpose of this PR?

Addresses part 2 of #718.

How did you implement your changes

Change the iteration in generate_and_save_{pixel/cell}_cluster_masks to a per-FOV basis, which requires deprecating the batch_size parameter.

generate_{pixel/cell}_cluster_mask will also support just a single fov string identifying which FOV to process (as opposed to a batch of FOVs).

Because save_fov_images and label_cells_by_cluster are used elsewhere in the repo, it's best to continue requiring a fovs list for these, and instead pass in a list with a single FOV from generate_{pixel/cell}_cluster_mask.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ngreenwald
Copy link
Member

Which other functions use save_fov_images and label_cells_by_cluster? Are we going to end up removing the batching functionality from them in the future? Or is it really worth keeping them as is?

@alex-l-kong
Copy link
Contributor Author

@ngreenwald label_cells_by_cluster is called in both the post-clustering notebook (via call to create_mantis_project) and the neighborhood analysis notebook, both of which pass in a list of FOVs. save_fov_images seems just limited to create_mantis_project, which also passes in a list of FOVs.

I thought about looping over each FOV in the create_mantis_project function, but this doesn't resolve the issue in the neighborhood analysis notebook. I have a few ideas about how to condense this to allow save_fov_images and create_mantis_project to work on just single FOVs, however it requires some refactoring to be done in the neighborhood analysis notebook itself. Not sure if this is within the scope of this PR, we can address that in a separate PR if need be.

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Okay agreed, that should be a separate PR. As you find stuff like that, go ahead and add it to the overall design doc so we can keep track of what else needs to be changed.

ark/utils/data_utils.py Outdated Show resolved Hide resolved
@ngreenwald ngreenwald merged commit 7d3741a into main Sep 27, 2022
@ngreenwald ngreenwald deleted the unbatch_mask_gen branch September 27, 2022 21:30
@srivarra srivarra added the enhancement New feature or request label Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants