add notebook for telco churn#542
Conversation
Signed-off-by: nina-xu <19981858+nina-xu@users.noreply.github.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
kendrickb-nvidia
left a comment
kendrickb-nvidia
left a comment
There was a problem hiding this comment.
A few thoughts, but overall looks great. Excited to get another demo and data setup up for the telco industry.
| "\n", | ||
| "full_df = pd.read_csv(DATA_PATH)\n", | ||
| "\n", | ||
| "# tmp: change NaN to None for Churn Reason\n", |
There was a problem hiding this comment.
nit: Can we just change this in the original data we use or otherwise make sure this is a non-issue?
| "source": [ | ||
| "### 📥 Load telco dataset and create train/holdout split\n", | ||
| "\n", | ||
| "Load the telco churn dataset from CSV, then create a deterministic 10,000-row subset for this tutorial.\n", |
There was a problem hiding this comment.
suggestion: This sounds like we're taking a 10k subset from some larger data, then splitting into 8000/2000. I don't think is accurate. Can we just say we make a 2k row test set and use the rest as training or something more generic and "never wrong" so less likely to need updating.
| "source": [ | ||
| "from nemo_safe_synthesizer.sdk.library_builder import SafeSynthesizer\n", | ||
| "\n", | ||
| "builder = SafeSynthesizer().with_data_source(train_real_df).with_train(num_input_records_to_sample=10000).with_generate(num_records=8000)\n", |
There was a problem hiding this comment.
nit: split this across multiple lines so easier to read and less likely to horizontally scroll.
| " \"Churn Score\",\n", | ||
| " \"Churn Reason\",\n", | ||
| " \"Count\",\n", | ||
| " \"Total Charges\",\n", |
There was a problem hiding this comment.
question: What's the reasoning on dropping these? Maybe good to have a comment about why? I see dropping CustomerID, Name, Zip Code, City, State, Country if we don't think the model will be able to use categoricals like that (and they wouldn't have signal ... maybe). And Churn Reason would give you a simple mapping to the label. But Count and Total Charges seem like useful signals for predicting if someone will continue with the telco or leave. Churn Value and Score seem like a holdover from how we generated the data, maybe we drop them from the dataset entirely?
| "execution_count": 21, | ||
| "id": "dded873c", | ||
| "metadata": {}, | ||
| "outputs": [ |
There was a problem hiding this comment.
nit: Do we want to clear all outputs? Or leave this one cell because it shows the quantitative results?
Perhaps could go in a markdown cell as Example results from a specific run or something? Seems odd to have all cell outputs cleared except one.
| "\n", | ||
| "print(\"Confusion matrices:\")\n", | ||
| "print(f\"Real model: {real_metrics['confusion_matrix']}\")\n", | ||
| "print(f\"Synth model: {synth_metrics['confusion_matrix']}\")" |
There was a problem hiding this comment.
suggestion: These aren't rendering in a particularly useful way right now. Need to pass through a heatmap plotter? Or maybe scikit has a method to show them nicely? Or make a dataframe with useful column and row labels and display() that?
| ], | ||
| "source": [ | ||
| "# Synthetic data and evaluation artifacts are automatically saved to the run directory.\n", | ||
| "print(f\"Artifacts saved to: {builder._workdir.generate.path}\")" |
There was a problem hiding this comment.
nit: Is this very helpful to a user? And is this not already shown in the output from the NSS .run() cell?
3 participants
Summary
Pre-Review Checklist
Ensure that the following pass:
make format && make checkor via prek validation.make testpasses locallymake test-e2epasses locallymake test-ci-containerpasses locally (recommended)/syncon this PR to trigger a run (auto-triggers on ready-for-review)Pre-Merge Checklist
Other Notes