-
Notifications
You must be signed in to change notification settings - Fork 0
Allow varying rank between cp and rise #31
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
- Coverage 77.79% 77.65% -0.14%
==========================================
Files 11 11
Lines 653 649 -4
==========================================
- Hits 508 504 -4
Misses 145 145
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:
|
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 allows varying rank between the initial decomposition and the final CP decomposition in the CC-PF2 workflow. The change enables more flexible model configurations by introducing a separate cp_rank
parameter that can differ from the main rank
parameter.
- Adds
cp_rank
parameter to control final CP decomposition rank independently - Refactors function naming from
calculateFMS
tocalculate_fms
for consistency - Updates standardization logic to handle varying matrix dimensions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
cellcommunicationpf2/utils.py | Adds cp_rank parameter to workflow function and renames FMS calculation function |
cellcommunicationpf2/cc_pf2.py | Implements cp_rank parameter and updates standardization for varying dimensions |
cellcommunicationpf2/figures/figure1.py | Updates function call to use new parameter |
cellcommunicationpf2/figures/figure2.py | Updates function imports to use renamed function |
Comments suppressed due to low confidence (1)
cellcommunicationpf2/cc_pf2.py:226
- Variable name 'signn' (with double 'n') appears to be a typo. It should be 'sign' or 'sign_vector' for clarity.
signn = np.sign(np.diag(factors[i][:k, :k]))
Co-authored-by: Copilot <[email protected]>
@aarmey The SVD step for the CP decomposition fails due to a memory issue (it wants to allocate some 1TB array). I am assuming this is fixed by providing the initialization to the method so I just wanted to confirm and double check if the init should be completely random or some baseline. |
@Nathaniel-github yep, random initialization is fine here. |
@aarmey Seems like I can't re-request the review, so just wanted to bump this PR. |
No description provided.