Conversation
There was a problem hiding this comment.
Pull request overview
Adds two new graph-denoising algorithms (adaptive_core_expansion, node_pls) to operate on pixelator.common.graph.Graph, along with new tests covering basic behavior and error cases.
Changes:
- Introduce
adaptive_core_expansiongraph partitioning algorithm (k-core seeded expansion + Bray–Curtis selection). - Introduce
node_plsutilities for neighborhood-expanded node abundance matrices and PLS regression. - Add test suites for both algorithms.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
src/pixelator/common/graph/adaptive_core_expansion.py |
New adaptive k-core expansion partitioning implementation; sets partition node attribute. |
src/pixelator/common/graph/node_pls.py |
New node-level PLS utilities for neighborhood abundance expansion, optional residualization, and model fitting. |
tests/common/test_adaptive_core_expansion.py |
Adds tests validating expected partitions on a known PXL component + invalid input checks. |
tests/common/test_node_pls.py |
Adds unit tests for residualization, neighborhood abundance matrix creation, and basic node_pls usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
maxkarlsson
left a comment
There was a problem hiding this comment.
As far as I can see it looks good, but perhaps it would be best if we try to calculate som matrices and PLS models here and compare them with R?
|
I have now added an additional parameter |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
maxkarlsson
left a comment
There was a problem hiding this comment.
I had a look at the PLS stuff and I think it looks great! I tried to see if I can find any differences to the R implementation but I cannot 👍
…ltiple denoise methods. Instead, we record number of nodes marked for removal by each method.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/pixelator/pna/analysis/denoise.py:423
denoise_one_core_layer()takesinflate_factor, but it is never forwarded toget_overexpressed_markers_in_one_core(). As a result, the CLI/analysis--inflate-factor/inflate_factorsetting has no effect on one-core denoising in this PR. Passinflate_factor=inflate_factorintoget_overexpressed_markers_in_one_core(and consider adding a small unit/integration assertion that changing inflate_factor changes the sampled removals).
markers_to_remove = get_overexpressed_markers_in_one_core(
node_marker_counts=node_marker_counts,
node_core_numbers=node_core_numbers,
pval_significance_threshold=pval_significance_threshold,
)
one_core_layer = node_marker_counts[
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| @@ -1,3 +1,3 @@ | |||
| version https://git-lfs.github.com/spec/v1 | |||
| oid sha256:f97f8fb13790711f0df6ab1f654ece94c994377b740488b34f9ba929039fb1e3 | |||
| oid sha256:9ea12d7216921248164d412b406aff86e5be0744bd87612b3ef19163e21b3e04 | |||
There was a problem hiding this comment.
What has changed here is the metadata pxl file metadata that switches panel name from proxiome-immuno-155 to proxiome-v1-immuno-155-v1.0
johandahlberg
left a comment
There was a problem hiding this comment.
This looks great. I have focused the review on readability and pythonisity (if that is a word). I have made inline comments where I think updates might be useful, but there is nothing here that blocks merging.
A high level comment is that I think the adaptive_core_expansion function could benefit from being split into multiple smaller subfunctions to improve the testability, e.g. one for each conceptual stage:
find_k_core_seedcreate_transition_probablity_matrixselect_largest_connected_componentfind_partition(called once perbinding_threhold)select_best_parition_candidate
Great work the whole team of @ptajvar, @maxkarlsson and @ludvigla in getting this over the finishing line so quickly!
| number_of_disqualified_components: int | None | ||
| ratio_of_disqualified_components: float | None |
There was a problem hiding this comment.
How come this is removed?
There was a problem hiding this comment.
A component would have been disqualified for having a k=1 layer that was larger than a given threshold. That only made sense for core-one denoising. Now that we multiple methods, a component may skip one denoising method but be denoised by another method. So instead of this, we store how many nodes are marked by each method to be removed from each component in the anndata.
Co-authored-by: Johan Dahlberg <johan@uppsala-bioinformatics.se>
…om new math module
Description
This PR adds two new algorithms (described in detail in pixelatorR-internal) for graph denoising. Both tools take a
Graphobject as input, including a networkx graph and a count matrix, and outputs the same object with additional node metrics which can be used for downstream filtering.This PR also adds additional options to the
denoisecli, enabling ACE and PLS denoising.Fixes: PNA-2591, and PNA-2671
Type of change
How Has This Been Tested?
Added tests in
tests/common/test_adaptive_core_expansion.pyandtests/common/test_node_pls.py.PR checklist:
pyproject.tomland cited it properly