Skip to content

fit() aliases caller-supplied fixed_W/fixed_H_expr/fixed_H_im arrays instead of copying (crash on read-only input, latent aliasing) #4

Description

@mkb-phoenix

Where: cellpie_main.py, intNMF.fit(), lines 190, 200, 212.

def fit(self, adata, fixed_H_expr=None, fixed_H_im=None, fixed_W=None):
    ...
    if fixed_W is None:
        self.theta, theta_it = self._HALS_W(...)
    else:
        self.theta = fixed_W          # line 190 — direct reference, no copy
        theta_it = 0
    ...
    if fixed_H_expr is not None:
        self.phi_expr = fixed_H_expr  # line 200 — same pattern
        self.phi_expr_it = 0
    ...
    if fixed_H_im is not None:
        self.phi_im = fixed_H_im      # line 212 — same pattern
        self.phi_im_it = 0

Symptom: fit() ends with in-place thresholding on its own factor attributes (self.phi_expr[self.phi_expr < 1e-10] = 0, etc). When a caller passes an already-fitted factor matrix in via fixed_H_expr/fixed_H_im/fixed_W — the documented mechanism for holding one factor fixed while fitting another sub-block, which is exactly how this package's own model_selection() bi-cross-validation is implemented internally — that in-place write can crash with ValueError: assignment destination is read-only, if the supplied array happens to be a read-only view (we hit this from a DataFrame.values extraction on one pandas version).

Root cause: self.theta = fixed_W etc. binds the attribute directly to whatever object the caller passed in, rather than copying it. Two distinct problems follow: (1) whether the crash above triggers depends on whether the caller's array happens to be writable, which is version/call-path dependent and not guaranteed either way by fit()'s own signature; (2) even when it doesn't crash, two separate intNMF instances that are each given the same array via fixed_* now alias the same underlying memory — one instance's later in-place threshold step would silently mutate data the other instance still holds a reference to.

Suggested fix: copy on assignment.

self.theta     = np.array(fixed_W,      copy=True)
self.phi_expr  = np.array(fixed_H_expr, copy=True)
self.phi_im    = np.array(fixed_H_im,   copy=True)

Scope note: _initialize_nmf() (lines 474-655) has its own, separate handling of the same fixed_W/fixed_H_expr/fixed_H_im parameters, which we have not touched or tested — flagging as unconfirmed/out of scope rather than claiming it's also affected.


Found while running CellPie's own cp_utils.model_selection() bi-cross-validation at production scale. Happy to share our patched fork if useful (commit pinned at 8880d673c, main branch).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions