-
Notifications
You must be signed in to change notification settings - Fork 36
[FEA] Add New Unsupervised Learning Example #371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Greptile SummaryAdded two new example scripts for unsupervised learning on smaller datasets like
The examples provide a complete workflow from unsupervised embedding learning to supervised classification, addressing issue #364's request for single-GPU examples on smaller datasets. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant mag_lp_mnmg as mag_lp_mnmg.py
participant GraphStore
participant Model
participant Output as Parquet Files
participant xgb as xgb.py
participant XGBoost
User->>mag_lp_mnmg: Run with torchrun
mag_lp_mnmg->>mag_lp_mnmg: Initialize distributed workers (NCCL, cuGraph, WholeGraph)
mag_lp_mnmg->>GraphStore: Load ogbn-mag dataset
mag_lp_mnmg->>GraphStore: Add nodes and edges (with reverse edges)
mag_lp_mnmg->>GraphStore: Calculate betweenness centrality
mag_lp_mnmg->>GraphStore: Add betweenness features to edges
mag_lp_mnmg->>Model: Create Classifier with Encoder/Decoder
mag_lp_mnmg->>Model: Train with LinkNeighborLoader
mag_lp_mnmg->>Model: Evaluate on test set
mag_lp_mnmg->>Model: Generate paper embeddings
mag_lp_mnmg->>Output: Export embeddings (x) and labels (y) to parquet
mag_lp_mnmg->>mag_lp_mnmg: Shutdown workers
User->>xgb: Run with --data_dir
xgb->>xgb: Create LocalCUDACluster
xgb->>Output: Read embeddings (x) and labels (y)
xgb->>xgb: Join data and split train/test
xgb->>XGBoost: Train multi-class classifier
xgb->>XGBoost: Evaluate on test set
xgb->>User: Display accuracy results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (4)
-
python/cugraph-pyg/cugraph_pyg/examples/mag_lp_mnmg.py, line 96 (link)logic:
global_rankis not defined in the scope of theClassifier.__init__methodTo fix this, you'll need to pass
global_rankas a parameter to the Classifier constructor. -
python/cugraph-pyg/cugraph_pyg/examples/mag_lp_mnmg.py, line 111 (link)logic:
global_rankis not defined in the scope of theClassifier.__init__methodTo fix this, you'll need to pass
global_rankas a parameter to the Classifier constructor. -
python/cugraph-pyg/cugraph_pyg/examples/mag_lp_mnmg.py, line 278 (link)logic:
global_rankis not defined in the scope of thetrainfunctionTo fix this, you'll need to pass
global_rankas a parameter to thetrainfunction. -
python/cugraph-pyg/cugraph_pyg/examples/mag_lp_mnmg.py, line 687-690 (link)logic: doubled the embedding instead of adding
x_paperresidual
2 files reviewed, 4 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
python/cugraph-pyg/cugraph_pyg/examples/mag_lp_mnmg.py, line 681-684 (link)logic: duplicates
x_dict["paper"]addition - should addx_paperresidual instead
2 files reviewed, 1 comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (3)
-
python/cugraph-pyg/cugraph_pyg/examples/mag_lp_mnmg.py, line 201 (link)style: missing
@torch.no_grad()decorator - inference should disable gradient computation for performance and memory efficiency -
python/cugraph-pyg/cugraph_pyg/examples/mag_lp_mnmg.py, line 643 (link)logic:
drop_last=Truecauses embeddings for the last batch to not be computed, leaving uninitialized values in the pre-allocated tensor at line 646-648 -
python/cugraph-pyg/cugraph_pyg/examples/mag_lp_mnmg.py, line 272 (link)style:
global_rankaccessed from global scope - consider passing it as a parameter for better function isolation and testabilityNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 files reviewed, 3 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (2)
2 files reviewed, 2 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (3)
-
python/cugraph-pyg/cugraph_pyg/examples/mag_lp_mnmg.py, line 548-549 (link)logic: Tensor used as index instead of scalar value.
train_szandtest_szare tensors but need to be converted to integers for slicing. -
python/cugraph-pyg/cugraph_pyg/examples/mag_lp_mnmg.py, line 492 (link)syntax: "betweeness" is misspelled
-
python/cugraph-pyg/cugraph_pyg/examples/xgb.py, line 92 (link)logic: Type consistency issue:
predictions_computedis a cupy array butdfy_test_computedis likely still a cudf Series/DataFrame
2 files reviewed, 3 comments
These are not isues - I've tested both files. |
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
tingyu66
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just left some nitpicks.
| pred_true_pos += ( | ||
| ((y_pred > 0.5).float() == 1.0) & (y_true.float() == 1.0) | ||
| ).sum() | ||
| pred_false_pos += ( | ||
| ((y_pred > 0.5).float() == 1.0) & (y_true.float() == 0.0) | ||
| ).sum() | ||
| pred_true_neg += ( | ||
| ((y_pred <= 0.5).float() == 1.0) & (y_true.float() == 0.0) | ||
| ).sum() | ||
| pred_false_neg += ( | ||
| ((y_pred <= 0.5).float() == 1.0) & (y_true.float() == 1.0) | ||
| ).sum() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need .float() here? Can it be simplified as ((y_pred > 0.5) & (y_true == 1)).sum()
| model, | ||
| optimizer, | ||
| wm_optimizer, | ||
| neg_ratio, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the negative ratio inside train() or test()?
Adds a new unsupervised learning example that can learn embeddings. Closes #364