Skip to content

fix: use predict_proba for probability metrics in estimator score - #1620

Open
Raashish Aggarwal (raashish1601) wants to merge 9 commits into
microsoft:mainfrom
raashish1601:fix/score-probability-metrics
Open

Raashish Aggarwal (raashish1601) wants to merge 9 commits into
microsoft:mainfrom
raashish1601:fix/score-probability-metrics

Conversation

@raashish1601

Copy link
Copy Markdown
Contributor

Why are these changes needed?

BaseEstimator.score(X, y, metric=...) (also used by AutoML.score) always used predict(), even for metrics that need class probabilities. As a result:

  • roc_auc / ap on a binary task were computed from hard 0/1 labels and gave a silently wrong value (e.g. 0.922 instead of 0.996 for an LGBM model on breast cancer);
  • log_loss, roc_auc_ovr etc. on a multiclass task raised errors (y_prob contains values greater than 1, y_score needs to be of shape (n_samples, n_classes)).

The search itself already picks between predict and predict_proba with get_y_pred() in flaml/automl/ml.py. score() now uses the same function. To keep passing extra keyword arguments through, get_y_pred() gets an optional **pred_kwargs that it forwards to predict/predict_proba. Existing callers are unchanged.

Related issue number

None. Found while checking AutoML.score with the metrics listed in its docstring.

Checks

  • I've used pre-commit to lint the changes in this PR (note the same in integrated in our CI checks).
  • I've included any doc changes needed for https://microsoft.github.io/FLAML/. No doc change needed; the docstring already lists these metrics.
  • I've added tests (if relevant) corresponding to the changes introduced in this PR. test/automl/test_estimator_score_metrics.py covers binary roc_auc and multiclass log_loss/roc_auc_ovr. All three fail on main and pass with this change. test/automl/test_score.py classification tests pass locally.
  • I've made sure all auto checks have passed.

@thinkall Li Jiang (thinkall) 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.

Overall review of the complete PR: approved.

Using the existing get_y_pred() metric dispatch in estimator scoring selects predict_proba() for probability metrics while retaining predict() for label metrics. Forwarding prediction kwargs preserves the existing score API, and the regression tests compare binary AUC and multiclass probability metrics with scikit-learn. No blocking issues found; the build matrix was still pending at review time.

Posted by thinkall-agent-auto-reviewer

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Binary ROC-AUC variants can receive invalid 2-D predictions, and the new test has a lint-breaking unused import.

2 open findings
What changed in this PR

Updates estimator scoring to use probabilities when required by classification metrics.

Changes:

  • Routes scoring predictions through get_y_pred.
  • Forwards prediction keyword arguments.
  • Adds binary and multiclass metric tests.
File Description
flaml/​automl/​model.py Selects predictions based on metric requirements.
flaml/​automl/​ml.py Adds prediction-argument forwarding.
test/​automl/​test_estimator_score_metrics.py Tests probability-based scoring.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread test/automl/test_estimator_score_metrics.py Outdated
Comment thread flaml/automl/ml.py

@thinkall Li Jiang (thinkall) 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.

Overall review of the complete updated PR: approved.

The post-review source commit corrects get_y_pred() so binary roc_auc_ovr, roc_auc_ovo and weighted variants receive positive-class probabilities instead of a two-column array, matching sklearn's binary ROC-AUC input. New regression cases cover all five binary variants while retaining multiclass probability checks. The original estimator.score() dispatch continues to use probabilities for probability metrics and labels for label metrics; the base refresh changes mape weighting separately and introduces no conflicting change to this dispatch. No blocking issue found in the updated PR.

Posted by thinkall-agent-auto-reviewer

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Standalone estimators using generic classification still route binary probability metrics incorrectly.

1 open issue
2 resolved since last review

Review effort: Balanced

Comment thread flaml/automl/model.py
if metric:
y_pred = self.predict(X_val, **kwargs)
# probability-based metrics such as roc_auc and log_loss need predict_proba
y_pred = get_y_pred(self, X_val, metric, self._task, **kwargs)

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.

This should be addressed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused fix has regression coverage and no unresolved findings; tests were not executed during this review.

1 open issue

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Standalone-estimator task compatibility remains unverified and needs human review.

1 open issue

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused changes address the reported scoring failures with regression coverage and no remaining actionable findings.

1 open issue

Review effort: Balanced

Comment thread flaml/automl/model.py
if metric:
y_pred = self.predict(X_val, **kwargs)
# probability-based metrics such as roc_auc and log_loss need predict_proba
y_pred = get_y_pred(self, X_val, metric, self._task, **kwargs)

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.

This should be addressed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

String tasks retained by directly fitted Spark classifiers cause probability-based scoring to fail.

2 open issues

Review effort: Balanced

Comment thread flaml/automl/model.py
if metric:
y_pred = self.predict(X_val, **kwargs)
# probability-based metrics such as roc_auc and log_loss need predict_proba
y_pred = get_y_pred(self, X_val, metric, self._task, **kwargs)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The previously reported probability-metric scoring issue for directly fitted estimators using task="classification" remains unresolved.

2 open issues

Review effort: Balanced

@thinkall Li Jiang (thinkall) 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.

Re-reviewed commits f507063f1d84ec43a4647d6d31c32a251a4da5d3 -> 304bb93119c0a317619fdde5b120c10067d1af82. The only delta is main's website/yarn.lock update of follow-redirects 1.16.0 -> 1.16.1. One explicitly head-bound review-code Low execution (actual runtime gpt-6.1-sol) found no new issue in this identical inherited patch. Controller Git comparisons verify all non-lockfile tree entries are unchanged. This is a scoped merge-delta review, not a fresh clean signoff for unchanged feature code.

Version/URL/caret/SRI shape checks pass. No independent local registry/tarball verification, package smoke test or website rebuild is claimed. Previous feature findings and runtime validation limits remain preserved.

At the 2026-10-26T12:13:44.943946+00:00 gate snapshot, actual current-head workflows were: docs completed/success; Build in_progress/no conclusion; Code formatting completed/success. Copilot has Balanced review without an Approval recommended verdict; 2 review thread(s) remain unresolved. Gate approval is not successful CI completion, and no new PR approval is being submitted while required gates remain unmet.

Posted by thinkall-agent-auto-reviewer

This branch has not been deployed

No deployments
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