-
Notifications
You must be signed in to change notification settings - Fork 36
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
Adding functionality to aggregate and annotate DeepProfiler output #78
Adding functionality to aggregate and annotate DeepProfiler output #78
Conversation
I added the real data tests, but the tests will fail until I add the data. The data will be added once broadinstitute/DeepProfilerExperiments#2 (comment) is resolved. |
@michaelbornholdt - one thing that you can add to this PR are numpy style docstrings (which are compatible with Sphinx) Example: pycytominer/pycytominer/aggregate.py Lines 26 to 57 in fdd0e09
If you're looking deeply at this code, I suspect that you'll need to figure this out on a per-argument basis anyway. Thanks! |
(once you accept the write access invitation, you'll be able to commit directly to this branch) |
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.
@gwaygenomics I have made some comments (mostly about docstrings) and have a couple of suggestions. Other than that everything looks good.
I also noticed that the PR template was missing. I realize that this PR is old but can it still be added?
Awesome @niranjchandrasekaran - this is great. @michaelbornholdt is still actively working on this PR, but you may have just saved him lots of time. In general, I love the idea of having you review this PR, since it technically will be a joint contribution between Michael and I. Michael, what do you think? I propose that we:
|
ooh, I see what happened now, I think once I marked the pull request as ready for review, the CODEOWNERS file triggered a review request:
In the future and to save Niranj time, I will not be so quick to mark a PR as ready-for-review |
Yeah, it's an old PR - but I agree adding it would be helpful. @michaelbornholdt, when you are happy with your fixes, please go through the checklist above. |
@gwaygenomics |
I also make sure to reset_index(drop=True) since we are merging metadata by index and this is easy to mixup
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
==========================================
- Coverage 98.64% 98.54% -0.11%
==========================================
Files 46 48 +2
Lines 1998 2197 +199
==========================================
+ Hits 1971 2165 +194
- Misses 27 32 +5
Continue to review full report at Codecov.
|
@niranjchandrasekaran - this is back to you. Thanks! |
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.
@gwaygenomics @michaelbornholdt LGTM! You may want to update the docstrings for the old functions in pycytominer/cyto_utils/load.py
while you are at it. But it can also wait. Nevertheless I have approved the PR.
awesome, thanks @niranjchandrasekaran !
I definitely think we should update docstrings as we go - but I think we did update these functions? Maybe I'm missing something or you mean a different file? Either way, that potential documentation update is relatively minor - which is great! @michaelbornholdt is this ready to merge? If so, I will go ahead. |
Ah, it was just a single function. pycytominer/pycytominer/cyto_utils/load.py Lines 7 to 13 in d2db947
|
All good! Go ahead. |
ooo, good catch. Thanks. I will update.
Wonderful. Can't wait! 😈 |
Description
In this pull request, we complete the integration of DeepProfiler output and pycytominer processing.
We discuss the implementation in broadinstitute/DeepProfilerExperiments#2.
What is the nature of your change?
Checklist
Please ensure that all boxes are checked before indicating that a pull request is ready for review.