-
Notifications
You must be signed in to change notification settings - Fork 0
Vary rank for CP and RISE #30
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
Conversation
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.
Pull Request Overview
This PR enables specifying a separate CP decomposition rank (cp_rank
) in the CC-PF2 workflow, refactors plotting and utility functions accordingly, and adds end-to-end figure generation tests.
- Introduce and propagate the
cp_rank
parameter throughcc_pf2
,run_cc_pf2_workflow
, and plotting scripts. - Add utility functions
resample
andcalculateFMS
for bootstrapping and factor match scoring. - Add tests to automatically run all figure scripts and update existing CC-PF2 tests to match the new output structure.
Reviewed Changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
pyproject.toml | Add tensorly-viz dependency for CP visualization |
cellcommunicationpf2/utils.py | Add resample and calculateFMS helper functions |
cellcommunicationpf2/tests/test_figures.py | New test to auto-run all figures (dynamic import logic) |
cellcommunicationpf2/tests/test_ccpf2.py | Update unpacking of cc_pf2 results to use the new cp_rank output |
cellcommunicationpf2/figures/figure1.py | Refactor Figure 1 generation with run_cc_pf2_workflow and new plotting helpers |
cellcommunicationpf2/figures/figure2.py | Update stability analysis to use resample and calculateFMS |
cellcommunicationpf2/figures/figure3.py | Add Figure 3 showing CP component weight vs. sample size |
cellcommunicationpf2/figures/commonFuncs/plotFactors.py | Improve row-color legend placement and fix hierarchical clustering |
cellcommunicationpf2/figures/common.py | Add run_cc_pf2_workflow , correct_conditions , and integrate CP rank logic |
cellcommunicationpf2/ccc.py | Simplify loops using _ and use inline conditionals |
cellcommunicationpf2/cc_pf2.py | Introduce cp_rank parameter, update return signature and CP logic |
Comments suppressed due to low confidence (3)
cellcommunicationpf2/utils.py:31
- [nitpick] Function name
calculateFMS
does not follow PEP8 snake_case conventions. Consider renaming it tocalculate_fms
for consistency.
def calculateFMS(A: anndata.AnnData, B: anndata.AnnData) -> float:
cellcommunicationpf2/utils.py:6
- The new utility functions
resample
andcalculateFMS
are not covered by any unit tests. Consider adding dedicated tests to verify correct stratified resampling and factor match score calculation.
def resample(data: anndata.AnnData, random_seed: int = None) -> anndata.AnnData:
cellcommunicationpf2/tests/test_figures.py:23
- The import_module call assumes the figures live under the
cellcommunicationpf2.tests.figures
package, but they are located incellcommunicationpf2/figures
. Update the import path tocellcommunicationpf2.figures.{module_name}
or adjust thepackage
argument accordingly.
module = importlib.import_module(f"..figures.{module_name}", package="cellcommunicationpf2.tests")
@Nathaniel-github I'll review this once we get your other PRs merged |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30 +/- ##
===========================================
+ Coverage 46.81% 78.14% +31.32%
===========================================
Files 6 11 +5
Lines 361 668 +307
===========================================
+ Hits 169 522 +353
+ Misses 192 146 -46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.