Updated ADMM and cytokine fact Stdev filtering

[nbedanova]

No description provided.

[Copilot]

🟡 Changes recommended

It introduces a non-portable default absolute CSV path (not present in-repo) and contains correctness/diagnostic mismatches (edge-case iteration crash and final stats computed from non-returned matrices).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

pf2rnaseq/factorization.py:388

• deconvolution_cytokine_admm introduces a new optimization routine but has no test coverage. Since pf2rnaseq/tests already validates reproducibility/outputs for other factorization code, it would be valuable to add a lightweight test on a small synthetic A that asserts output shapes, diagonal constraints on Z_W, and non-negativity when enabled.

def deconvolution_cytokine_admm(
    A: np.ndarray,
    alpha_h: float = 0.1,
    alpha_w: float = 0.01,
    rho_w_init: float | None = None,
    rho_h_init: float | None = None,
    max_iter: int = 10000,
    tol_abs: float = 1e-4,
    tol_rel: float = 1e-3,
    random_state: int = 1,
    adaptive_rho: bool = True,
    rho_bounds: tuple[float, float] = (1e-4, 1e4),
    non_negative_w: bool = True,
    non_negative_h: bool = True,
) -> tuple[np.ndarray, np.ndarray, dict]:

• Files reviewed: 1/1 changed files
• Comments generated: 6
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are a few concrete portability/correctness issues (hard-coded paths, indexing consistency, RNG side effects, and missing alignment validation) that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

pf2rnaseq/factorization.py:49

• This helper hard-codes a machine-specific absolute CSV path, and it returns 0-indexed component IDs even though most component-facing helpers in this repo take 1-indexed component numbers (and index arrays with cmp - 1). Computing the default path relative to the package and returning 1-indexed IDs by default will make this easier to use and avoid off-by-one mistakes.

def filter_components_by_cytokine_quantile(
    threshold: float,
    csv_path: str = "/home/nicoleb/Pf2-scRNAseq-1/pf2rnaseq/Data/donor_vs_cytokine_variability_sd.csv",
) -> np.ndarray:

pf2rnaseq/factorization.py:380

• New public helpers in factorization.py (cytokine quantile filtering, ADMM deconvolution, and correct_conditions_from_npy) aren’t covered by tests; existing tests already exercise this module, so adding at least lightweight shape/invariant checks would help prevent regressions.

def deconvolution_cytokine_admm(
    A: np.ndarray,
    alpha_h: float = 0.1,
    alpha_w: float = 0.01,
    rho_w_init: float | None = None,
    rho_h_init: float | None = None,
    max_iter: int = 10000,

• Files reviewed: 22/23 changed files
• Comments generated: 3
• Review effort level: Lite