Skip to content

Fix GRASP's dead similar-patient graph - #1194

Open
fbonc wants to merge 2 commits into
sunlabuiuc:masterfrom
fbonc:grasp-fix
Open

Fix GRASP's dead similar-patient graph#1194
fbonc wants to merge 2 commits into
sunlabuiuc:masterfrom
fbonc:grasp-fix

Conversation

@fbonc

@fbonc fbonc commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Problem
GRASPLayer set self.A_mat = None in __init__ and never reassigned it, so the if self.A_mat is None branch always won and the GCN was fed an identity adjacency mat. The kNN "similar-cluster" graph (which is the whole point of GRASP) was dead code.

The kNN branch also hardcoded n_neighbors=20 while the graph has only cluster_num nodes (default 2), so it would have raised n_neighbors > n_samples had it ever run. The dead-code guard prevented the crash-prone call from ever executing.

Fix
Build the kNN graph unconditionally, capping neighbors to the node count with k = min(20, cluster_num - 1), and keep identity only as a genuine fallback when there are too few clusters (k < 1). Removed the unused self.A_mat (the graph is rebuilt each forward from the current batch's cluster centers, so it can't be cached anyway).

Tests
Added a regression test that looks at the adjacency passed to the GCN. it must be a real kNN graph (off-diagonal edges, no self-loops) for cluster_num >= 2, and identity only for cluster_num == 1. Fails on the old identity-only behavior, passes on the fix.

@lehendo lehendo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me!

@DarylOkeke

DarylOkeke commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

The kNN graph now runs, but the adjacency passed to the GCN is missing the self-connections from GRASP. The paper augments the graph as A_hat = A + I before graph convolution; this code uses include_self=False and passes A directly, and the new test requires a zero diagonal. That makes each center aggregate only neighbors (with the default two clusters, each center gets only the other center). Could we add the identity before the GCN and update the test? Paper: https://cdn.aaai.org/ojs/16152/16152-13-19646-1-2-20210518.pdf

@lehendo

lehendo commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

@fbonc pls address Daryl's comment.

@fbonc

fbonc commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

The kNN graph now runs, but the adjacency passed to the GCN is missing the self-connections from GRASP. The paper augments the graph as A_hat = A + I before graph convolution; this code uses include_self=False and passes A directly, and the new test requires a zero diagonal. That makes each center aggregate only neighbors (with the default two clusters, each center gets only the other center). Could we add the identity before the GCN and update the test? Paper: https://cdn.aaai.org/ojs/16152/16152-13-19646-1-2-20210518.pdf

Fixed. implementation now adds the identity matrix to the kNN adjacency before it is passed to both GCN layers. also updated the regression test to assert self-connections on the diagonal while retaining the check that neighbor edges are present.

@DarylOkeke

Copy link
Copy Markdown
Contributor

Looks good now. The kNN graph includes the self-connections required by the paper.

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.

3 participants