feat(evaluation): add automatic visualization for model evaluation - #1
raofal-msodeh wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThese changes add optional visualization support to the ModelEvaluator class, enabling automatic generation and saving of plots (confusion matrix, ROC curve for binary classification, and residuals for regression) when enabled via a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
mltools/evaluation/evaluator.py (2)
47-47: Make the plots directory configurable.Hardcoding
'plots'ties the output location to the CWD of whichever process imports the evaluator. Surfacing it throughConfig(e.g.self.config.evaluation.get('plots_dir', 'plots')) or as a constructor argument would let users redirect output for tests, notebooks, or pipelines without monkey-patching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mltools/evaluation/evaluator.py` at line 47, The plots directory is hardcoded via self.plots_dir = 'plots'; make it configurable by reading from the Evaluator constructor or Config: update the Evaluator.__init__ (or the class constructor that sets self.plots_dir) to accept an optional plots_dir parameter and/or pull from self.config.evaluation.get('plots_dir', 'plots') and assign that value to self.plots_dir so callers (or tests/pipelines) can override the output location without monkey-patching.
150-193: Default filenames silently overwrite prior plots across evaluations.
plot_confusion_matrix,plot_roc_curve, andplot_residualsall default to a fixed filename. If a user evaluates several models (or compares a regression and a re-run) in the same session/CWD, each call clobbers the previous PNG with no warning. Consider including a caller-supplied tag (model name / timestamp) in the default, or at least documenting the overwrite behavior.🛠️ Sketch
- def plot_confusion_matrix(self, cm: np.ndarray, filename: str = 'confusion_matrix.png'): + def plot_confusion_matrix(self, cm: np.ndarray, filename: Optional[str] = None): """Plot and save confusion matrix""" + filename = filename or 'confusion_matrix.png' + self._ensure_plots_dir() plt.figure(figsize=(10, 7)) ...Same treatment for the other two helpers; alternatively expose a
tagparameter on the publicevaluate_*methods that gets threaded through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mltools/evaluation/evaluator.py` around lines 150 - 193, The three plotting helpers (plot_confusion_matrix, plot_roc_curve, plot_residuals) currently use fixed default filenames and silently overwrite prior plots; change their signatures to accept an optional tag: str = None (or accept filename: Optional[str] = None) and when no explicit filename is provided, build a unique filename by appending a caller-supplied tag if given or a timestamp (e.g. YYYYmmdd_HHMMSS) to the base name before saving; update callers (or public evaluate_* methods if present) to thread a tag through to these functions so multi-run outputs do not clobber each other and ensure the logger message uses the final path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mltools/evaluation/evaluator.py`:
- Around line 105-108: The code assumes y_pred_proba is 2-D (using
y_pred_proba[:, 1]) which breaks if callers pass a 1-D array of positive-class
probabilities; update evaluate_classification (and any ROC/AUC sites) to
normalize y_pred_proba once: if y_pred_proba is 1-D treat it as the
positive-class probabilities, otherwise extract column 1 from a 2-D array, store
that normalized 1-D array (e.g., pos_probs) and use pos_probs for ROC/AUC calls
and for plot_roc_curve to avoid IndexError and duplicate logic; reference
symbols: evaluate_classification, y_pred_proba, plot_roc_curve, and any ROC/AUC
computation lines.
- Around line 45-49: The constructor ModelEvaluator.__init__ currently creates
the 'plots' directory unconditionally via self.plots_dir — defer this side
effect: remove the os.path.exists/os.makedirs calls from __init__, keep
self.plots_dir = 'plots', add a helper method _ensure_plots_dir that calls
os.makedirs(self.plots_dir, exist_ok=True), and call self._ensure_plots_dir() at
the start of each plotting helper (any methods named plot_*). This ensures the
directory is created only when a plot is actually saved and avoids the TOCTOU
race by using exist_ok=True.
---
Nitpick comments:
In `@mltools/evaluation/evaluator.py`:
- Line 47: The plots directory is hardcoded via self.plots_dir = 'plots'; make
it configurable by reading from the Evaluator constructor or Config: update the
Evaluator.__init__ (or the class constructor that sets self.plots_dir) to accept
an optional plots_dir parameter and/or pull from
self.config.evaluation.get('plots_dir', 'plots') and assign that value to
self.plots_dir so callers (or tests/pipelines) can override the output location
without monkey-patching.
- Around line 150-193: The three plotting helpers (plot_confusion_matrix,
plot_roc_curve, plot_residuals) currently use fixed default filenames and
silently overwrite prior plots; change their signatures to accept an optional
tag: str = None (or accept filename: Optional[str] = None) and when no explicit
filename is provided, build a unique filename by appending a caller-supplied tag
if given or a timestamp (e.g. YYYYmmdd_HHMMSS) to the base name before saving;
update callers (or public evaluate_* methods if present) to thread a tag through
to these functions so multi-run outputs do not clobber each other and ensure the
logger message uses the final path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 712977b3-1850-43e4-a76b-01defae49a4b
📒 Files selected for processing (2)
README.mdmltools/evaluation/evaluator.py
|
|
||
| # Create plots directory if it doesn't exist | ||
| self.plots_dir = 'plots' | ||
| if not os.path.exists(self.plots_dir): | ||
| os.makedirs(self.plots_dir) |
There was a problem hiding this comment.
Avoid creating plots/ as an unconditional side effect of __init__.
The directory is created on every ModelEvaluator() instantiation, even when the caller never sets plot=True. This pollutes the current working directory (which is whatever the user happens to be running from) and surprises consumers who only want metrics. Defer creation to the point where a plot is actually being saved, and use exist_ok=True to avoid the TOCTOU between os.path.exists and os.makedirs.
🛠️ Proposed fix
def __init__(self, config: Optional[Config] = None):
...
self.config = config or Config()
self.logger = get_logger('ModelEvaluator')
self.results = {}
-
- # Create plots directory if it doesn't exist
- self.plots_dir = 'plots'
- if not os.path.exists(self.plots_dir):
- os.makedirs(self.plots_dir)
+ self.plots_dir = 'plots'
+
+ def _ensure_plots_dir(self) -> None:
+ os.makedirs(self.plots_dir, exist_ok=True)Then call self._ensure_plots_dir() at the start of each plot_* helper before computing the path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mltools/evaluation/evaluator.py` around lines 45 - 49, The constructor
ModelEvaluator.__init__ currently creates the 'plots' directory unconditionally
via self.plots_dir — defer this side effect: remove the
os.path.exists/os.makedirs calls from __init__, keep self.plots_dir = 'plots',
add a helper method _ensure_plots_dir that calls os.makedirs(self.plots_dir,
exist_ok=True), and call self._ensure_plots_dir() at the start of each plotting
helper (any methods named plot_*). This ensures the directory is created only
when a plot is actually saved and avoids the TOCTOU race by using exist_ok=True.
| if plot: | ||
| self.plot_confusion_matrix(cm) | ||
| if y_pred_proba is not None and len(np.unique(y_true)) == 2: | ||
| self.plot_roc_curve(y_true, y_pred_proba[:, 1]) |
There was a problem hiding this comment.
y_pred_proba[:, 1] assumes a 2-D probability array.
If a caller passes a 1-D array of positive-class probabilities for a binary problem (a common shape returned by some estimators / wrappers), line 108 will raise IndexError: too many indices for array. The ROC AUC computation on line 82 has the same issue, but here the failure happens after evaluate_classification has already logged metrics, which is more confusing. Consider normalizing the shape once and reusing it.
🛠️ Proposed fix
- if plot:
- self.plot_confusion_matrix(cm)
- if y_pred_proba is not None and len(np.unique(y_true)) == 2:
- self.plot_roc_curve(y_true, y_pred_proba[:, 1])
+ if plot:
+ self.plot_confusion_matrix(cm)
+ if y_pred_proba is not None and len(np.unique(y_true)) == 2:
+ proba_pos = y_pred_proba[:, 1] if y_pred_proba.ndim == 2 else y_pred_proba
+ self.plot_roc_curve(y_true, proba_pos)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mltools/evaluation/evaluator.py` around lines 105 - 108, The code assumes
y_pred_proba is 2-D (using y_pred_proba[:, 1]) which breaks if callers pass a
1-D array of positive-class probabilities; update evaluate_classification (and
any ROC/AUC sites) to normalize y_pred_proba once: if y_pred_proba is 1-D treat
it as the positive-class probabilities, otherwise extract column 1 from a 2-D
array, store that normalized 1-D array (e.g., pos_probs) and use pos_probs for
ROC/AUC calls and for plot_roc_curve to avoid IndexError and duplicate logic;
reference symbols: evaluate_classification, y_pred_proba, plot_roc_curve, and
any ROC/AUC computation lines.
1 participant
Summary by CodeRabbit
New Features
plots/directory.Documentation