[enhancement] add sklearnex version of validate_data, _check_sample_weight
#2177
Conversation
I am purposefully not including this in any estimators at this point to speed the review/merging of the PR: there will be performance benchmarks for #2209 #2207 #2206 #2201 and #2189. Good question. GPU performance improvements will occur when array_api support in the dispatch function is included. (so unfortunately not yet, there is some aspects missing to those PRs which must come from #2096) |
|
/intelci: run |
|
/intelci: run |
sklearnex/tests/utils/base.py
Outdated
| X_table, sua_iface=sua_iface, sycl_queue=X.sycl_queue, xp=xp | ||
| ) | ||
| self.y_attr_ = from_table( | ||
| y_table, sua_iface=sua_iface, sycl_queue=X.sycl_queue, xp=xp |
There was a problem hiding this comment.
Is it Ok that y_attr_ goes with X's queue?
Also, what happens if X and y are from different namespaces and have different sua_iface? For example, X - from dpnp and y - from numpy. What is expected to happen in this case?
| y_table, sua_iface=sua_iface, sycl_queue=X.sycl_queue, xp=xp | |
| y_table, sua_iface=sua_iface, sycl_queue=y.sycl_queue, xp=xp |
There was a problem hiding this comment.
This was a failure in the original implementation, though the backend in sklearnex is very fuzzy in this (not standard). For questions about from_table I would ask @samir-nasibli.
| if dispatch: | ||
| assert type(X) == type( | ||
| X_array | ||
| ), f"validate_data converted {type(X)} to {type(X_array)}" | ||
| assert type(X) == type(X_out), f"from_array converted {type(X)} to {type(X_out)}" |
There was a problem hiding this comment.
I guess y needs to be checked here and in 'else' branch as well.
|
/intelci: run |
Are we expecting perf to be on par with what we were seeing from #2153 for the respective algorithms once the other PRs are finalized? |
|
/intelci: run |
|
/intelci: run |
|
/intelci: run |
Co-authored-by: ethanglaser <42726565+ethanglaser@users.noreply.github.com>
|
/intelci: run |
Description
This is another interim PR towards introducing the new onedal finiteness checker into the sklearnex estimator workflows. This is not yet introduced into any of the estimators, and so performance benchmarks are not necessary. This PR focuses on making sure that input and outputs of
validate_dataand_check_sample_weightare respected for sycl_usm_ndarray types and that the new finite checker is properly called and yields results in a range of scenarios. This is also done to minimize the review burden, as changing all the estimators is a large change.The new process for all estimators will be as follows:
validate_dataand_check_sample_weightonce in sklearnex in_onedal_*methods called by device_offload's dispatchvalidate_dataor_check_sample_weightunless an operation before the oneDAL backend can yield a inf/NaN (this is a strict condition, and is expected to be extremely uncommon/ hard to allow)onedalorsklearnexfolders must have assert_all_finite checks turned off.A follow up PR will create a design test for this, and will introduce the new validate_data in one estimator. Other estimators will occur in individual PRs due to the depth of the changes.
PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.
You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance