-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
[pyspark] rework transform to reuse same code #9292
Conversation
@WeichenXu123 @trivialfis could you please help to review this PR? Previously, SparkXGBClassifier totally overrides the whole _transform function which causes duplicated common code, so this PR tries to unify them. |
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.
Does the refactor make the code cleaner or easier to understand? I find it quite hacky, but might be a general issue with PySpark-based libraries.
result = data[pred.prediction] | ||
if pred_contrib_col_name: | ||
contribs = pred_contribs(model, X, base_margin) | ||
data[pred.pred_contrib] = pd.Series(list(contribs)) |
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.
Not sure how it works. Wouldn't this be super slow?
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.
any thoughts to rework this? using contribs.tolist() ?
python-package/xgboost/spark/core.py
Outdated
def _post_transform(self, dataset: DataFrame, pred_col: Column) -> DataFrame: | ||
"""Post process of transform""" | ||
prediction_col_name = self.getOrDefault(self.predictionCol) | ||
single_pred = "," not in self._out_schema() |
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.
That's a bit, hmm, unconventional. Is there a way to refactor this to make it more "standard"?
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.
The _out_schema has typing hint with str returned which is a DDL formatted string, so if there're many columns, it must have at least a ",". So it's conventional when we check if it is a single column according to if "," is in the schema.
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.
We are meta-programming by manipulating strings here.
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.
Maybe just use the pred_contrib_col_name
as a predicate?
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.
fixed this issue.
python-package/xgboost/spark/core.py
Outdated
pred_contrib_col_name = self._get_pred_contrib_col_name() | ||
|
||
def _predict( | ||
model: XGBModel, X: ArrayLike, base_margin: Optional[np.ndarray] |
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.
base_margin
is not necessarily np.ndarray
. Let's stick with ArrayLike
.
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.
Done
python-package/xgboost/spark/core.py
Outdated
def _post_transform(self, dataset: DataFrame, pred_col: Column) -> DataFrame: | ||
"""Post process of transform""" | ||
prediction_col_name = self.getOrDefault(self.predictionCol) | ||
single_pred = "," not in self._out_schema() |
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.
Maybe just use the pred_contrib_col_name
as a predicate?
62aebbd
to
c8d353b
Compare
To fix #9170