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

Implementing DeepProfiler single cell output and normalization #210

Merged
merged 22 commits into from
Jun 28, 2022

Conversation

roshankern
Copy link
Member

Description

As of commit e1091b3, DeepProfiler saves features in the format features/plate/well/site.npz This change allows pycytominer.cyto_utils.DeepProfilerData to work with this format with filename_delimiter = "/". A DeepProfilerData object can be used with AggregateDeepProfiler to aggregate the data (as in current version of pycytominer) or with SingleCellDeepProfiler to load and normalize single cell embeddings.

What is the nature of your change?

  • Bug fix (fixes an issue).
  • Enhancement (adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • This change requires a documentation update.

Checklist

Please ensure that all boxes are checked before indicating that a pull request is ready for review.

  • I have read the CONTRIBUTING.md guidelines.
  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • New and existing unit tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have deleted all non-relevant text in this pull request template.

@gwaybio This is ready for review.

Copy link
Member

@gwaybio gwaybio left a comment

Choose a reason for hiding this comment

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

Looks great - I'm excited to have this functionality in pycytominer!

I have only one feature request. Can you add the functionality to output the non-normalized single cell features inside setup_normalize()? (I've suggested a rename to get_singlecells())

I've made several suggestions to making this change. I haven't tested these suggestions so please make sure that I've thought all the logic through.

I've also just now kicked off the continuous integration tests. If these pass then the only change required for the test suite is to add a test for the new output single cell functionality.

pycytominer/cyto_utils/DeepProfiler_processing.py Outdated Show resolved Hide resolved
pycytominer/cyto_utils/DeepProfiler_processing.py Outdated Show resolved Hide resolved
pycytominer/cyto_utils/DeepProfiler_processing.py Outdated Show resolved Hide resolved
roshankern and others added 8 commits June 28, 2022 09:25
Co-authored-by: Gregory Way <gregory.way@gmail.com>
Co-authored-by: Gregory Way <gregory.way@gmail.com>
Co-authored-by: Gregory Way <gregory.way@gmail.com>
Co-authored-by: Gregory Way <gregory.way@gmail.com>
Co-authored-by: Gregory Way <gregory.way@gmail.com>
@roshankern
Copy link
Member Author

Awesome @gwaybio! The changes in 836bf1e should resolve the integration test failures.

Copy link
Member

@gwaybio gwaybio left a comment

Choose a reason for hiding this comment

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

Congrats on the successful integration tests!

Two other comments, see below. Then I think we can merge.

pycytominer/cyto_utils/DeepProfiler_processing.py Outdated Show resolved Hide resolved
pycytominer/cyto_utils/DeepProfiler_processing.py Outdated Show resolved Hide resolved
Comment on lines 43 to 44
single_cells = single_cells_DP.get_single_cells(output=True)
single_cells_normalized = single_cells_DP.normalize_deep_single_cells(
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test where you apply normalize() to single_cells as described in 43 using the exact same normalize logic used in .normalize_deep_single_cells() and pd.DataFrame.assert_equal?

It would be good to test the internal normalize logic has expected behavior as an external application.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately I had to do a bit of strangling with the locations to make the DP data compatible with normalize(). Let me know if the changes in b754251 work for the pandas assertion.

roshankern and others added 2 commits June 28, 2022 10:23
Co-authored-by: Gregory Way <gregory.way@gmail.com>
Co-authored-by: Gregory Way <gregory.way@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2022

Codecov Report

Merging #210 (34b4aba) into master (5f9a774) will decrease coverage by 0.23%.
The diff coverage is 91.96%.

@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
- Coverage   95.74%   95.51%   -0.24%     
==========================================
  Files          53       53              
  Lines        2611     2695      +84     
==========================================
+ Hits         2500     2574      +74     
- Misses        111      121      +10     
Flag Coverage Δ
unittests 95.51% <91.96%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pycytominer/cyto_utils/load.py 83.60% <66.66%> (-4.40%) ⬇️
pycytominer/cyto_utils/DeepProfiler_processing.py 92.96% <91.22%> (-3.63%) ⬇️
pycytominer/cyto_utils/__init__.py 100.00% <100.00%> (ø)
...ts/test_cyto_utils/test_DeepProfiler_processing.py 100.00% <100.00%> (ø)
pycytominer/tests/test_cyto_utils/test_load.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@gwaybio gwaybio changed the title Deepprofiler implementation Implementing DeepProfiler single cell output and normalization Jun 28, 2022
@gwaybio gwaybio merged commit eec910a into cytomining:master Jun 28, 2022
@roshankern roshankern deleted the deepprofiler-implementation branch June 28, 2022 19:46
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.

3 participants