-
Notifications
You must be signed in to change notification settings - Fork 72
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
Handle nullable types and empty partitions before Dask-ML predict #799
Conversation
Codecov Report
@@ Coverage Diff @@
## main #799 +/- ##
==========================================
- Coverage 74.88% 74.54% -0.35%
==========================================
Files 71 71
Lines 3588 3630 +42
Branches 748 759 +11
==========================================
+ Hits 2687 2706 +19
- Misses 771 786 +15
- Partials 130 138 +8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -183,7 +184,16 @@ def convert(self, rel: "LogicalPlan", context: "dask_sql.Context") -> DataContai | |||
|
|||
delayed_model = [delayed(model.fit)(x_p, y_p) for x_p, y_p in zip(X_d, y_d)] | |||
model = delayed_model[0].compute() | |||
model = ParallelPostFit(estimator=model) | |||
if "sklearn" in model_class: | |||
output_meta = np.array([]) |
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.
@VibhuJawa I wanted to ask your opinion on this check?
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.
I think we should handle this in https://github.com/dask-contrib/dask-sql/pull/832/files PR and follow dask/dask-ml#912 for a clean fix. I dont think we should fix it here.
OK, I've moved these changes into #832. |
@VibhuJawa
Re-opening #783 here.