Skip to content

changes min_allowed_nodes_pct in adaptive_core_expansion to 0.9#404

Open
ludvigla wants to merge 4 commits into
devfrom
pna-3083
Open

changes min_allowed_nodes_pct in adaptive_core_expansion to 0.9#404
ludvigla wants to merge 4 commits into
devfrom
pna-3083

Conversation

@ludvigla

@ludvigla ludvigla commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Description

In samples with extremely low connectivity, ACE can be quite aggressive and remove >10% of UMIs. For PBMC samples, this is quite bad.

This PR changes the default value for min_allowed_nodes_pct from 0.8 to 0.9 in adaptive_core_expansion. This means that we will not allow ACE to remove more than 10% of UMIs. On the downside, it will not select the best possible denoising solution. For e.g. MOLT4, we should expect higher noise levels with this change.

For the majority of experiments which are PBMC, it's probably better to be cautious and keep more data in the graphs.

Fixes: PNA-3083

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Updated tests in tests/pna/denoise/test_denoise.py

Note that I added a second component (REFERENCE_ACE_COMPONENT_HIGH_QUALITY) to make sure that we test ACE on a component that actually gets denoised. The previous component (REFERENCE_ACE_COMPONENT still used in other tests) didn't get denoised at all because ACE failed to find a partition where the "high" core layer was > 90% of nodes (according to our new min_allowed_nodes_pct threshold at 0.9).

PR checklist:

  • This comment contains a description of changes (with reason).
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • If a new tool or package is included, I have updated dependencies in pyproject.toml and cited it properly
  • I have checked my code and documentation and corrected any misspellings
  • I have documented any significant changes to the code in CHANGELOG.md

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the default ACE partition-selection constraint in adaptive_core_expansion so the chosen “high” core must contain at least 90% of nodes, limiting how aggressively ACE can mark nodes (UMIs) as peripheral.

Changes:

  • Change adaptive_core_expansion(..., min_allowed_nodes_pct=...) default from 0.8 to 0.9.
Comments suppressed due to low confidence (1)

src/pixelator/pna/graph/adaptive_core_expansion.py:264

  • The PR description indicates this change requires a documentation update and frames the behavior as limiting ACE to removing at most 10% of UMIs. In this file, the public docstring still describes the constraint generically as a node fraction, without noting the new default (0.9) or that nodes correspond to UMIs in PNA graphs. Consider updating the docstring to reflect the new default behavior so the rationale is discoverable to API users.
    min_allowed_nodes_pct: float = 0.9,
    select_lcc: bool = True,
) -> Graph:
    """Perform Adaptive Core Expansion (ACE) graph partitioning.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants