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 cell table batching functionality #726

Merged
merged 11 commits into from
Sep 27, 2022
Merged

Conversation

alex-l-kong
Copy link
Contributor

What is the purpose of this PR?

Addresses stage 1 of #720, removing batching of generate_cell_table.

How did you implement your changes

The batch loop inside generate_cell_table has been changed to a per-FOV iteration.

The call in 1_Segment_Image_Data.ipynb needed to be updated accordingly.

Remaining issues

In particular, changes were made to the way fovs and filenames were being handled in generate_cell_table. MIBItiff support requires us to retrieve filenames, however for non-MIBItiffs the loop needs to be more fully guarded against filenames being set to None (which happens when you call list_files on a folder structure).

We will not have to worry about this once MIBItiff support gets phased out.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

…rate_cell_table is tested on different-sized FOVs
@alex-l-kong
Copy link
Contributor Author

@benoberlton this PR should address the error caused by variable image sizes you were getting during generate_cell_table. Let me know if any issues pop up.

@alex-l-kong alex-l-kong requested review from ngreenwald and removed request for ngreenwald September 25, 2022 00:02
@alex-l-kong
Copy link
Contributor Author

@ngreenwald realized I needed to update the documentation for create_marker_count_matrices to indicate that the FOV dimensions should support just a single FOV, however seems I can't push the change until you review the rest of the changes (even if I remove the review request). If all looks good I'll push that change separately.

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.

Huh that's strange, I didn't know review status could block subsequent changes. Won't have time to look tonight, I'll submit this which will hopefully address the issue

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.

Looks good, just need to add back in the code that was removed. Seems like this has happened a bunch of times on your PRs, I think you said your editor does it automatically?

ark/segmentation/marker_quantification.py Outdated Show resolved Hide resolved
@alex-l-kong
Copy link
Contributor Author

Looks good, just need to add back in the code that was removed. Seems like this has happened a bunch of times on your PRs, I think you said your editor does it automatically?

@ngreenwald yeah, looks like I'll need to officially need to retire my merge tool lol, Sublime Merge hasn't been the easiest to interface with.

@alex-l-kong
Copy link
Contributor Author

@ngreenwald this should resolve generate_cell_table with main.

@ngreenwald ngreenwald merged commit ca7e5ab into main Sep 27, 2022
@ngreenwald ngreenwald deleted the unbatch_cell_table branch September 27, 2022 00:08
@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