Refactor eigdecomp phasing and sorting#397
Open
nvictus wants to merge 4 commits intoopen2c:masterfrom
Open
Conversation
gfudenberg
reviewed
Oct 17, 2022
| ignore_diags=ignore_diags, | ||
| clip_percentile=99.9, | ||
| sort_metric=None, | ||
| ) |
Member
There was a problem hiding this comment.
is this desired? this issue suggests to expose sort_metric #311
Member
Author
There was a problem hiding this comment.
I removed it to be replaced with corr_metric (used for both flipping and reordering) and reorder instead.
Member
There was a problem hiding this comment.
aha, gotcha-- thoughts on phasing_metric or phasing_corr_metric to help remind the user what this is for?
golobor
reviewed
Oct 17, 2022
|
|
||
| # Go through eigendecomposition results and fill in output tables. | ||
| for _region, _eigvals, _eigvecs in results: | ||
| idx = bioframe.select(eigvec_table, _region).index |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
sergpolly
requested changes
Apr 19, 2023
Member
sergpolly
left a comment
sergpolly
left a comment
There was a problem hiding this comment.
- imho it'd be nice to align cis and trans function a bit - to minimize confusion
- cli argument regarding sorting/phasing needs to be sorted out
other than that seems to be a useful refactoring !
|
|
||
| # Go through eigendecomposition results and fill in output tables. | ||
| for _region, _eigvals, _eigvecs in results: | ||
| idx = bioframe.select(eigvec_table, _region).index |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
sergpolly
added a commit
to sergpolly/cooltools
that referenced
this pull request
Nov 24, 2023
few tweaks and reorderings in eigdecomp to address my comments in open2c#397
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Some work to modularize various sub-operations in eigdecomp.
Decouple target matrix preparation from decomposition. Might make it easier to provide different target matrix options (e.g. balanced vs unbalanced O/E, mean-centered vs 1-centered, fake-cis vs other approach).
Decouple deterministic sign flipping (providing
phasing_track) and reordering/sorting (reorder=True). Both options will make use ofcorr_metricif activated, which defaults topearsonr.