[enhancement] refactor onedal to_table
#2151
Conversation
|
/intelci: run |
|
/intelci: run |
onedal/datatypes/_data_conversion.py
Outdated
| arrays, sycl_usm_ndarrays, and scalars. Tables will use pointers to the | ||
| original array data. Scalars will be copies. Arrays may be modified in- | ||
| place by oneDAL during computation. This works for data located on CPU and | ||
| SYCL-enabled Intel GPUs. Only singular datatypes are allowed. |
There was a problem hiding this comment.
The term singular datatype looks strange to me. Is it possible to come up with a more understandable wording? 'numeric data types', 'integer and floating point data types' - something like that.
Or maybe I didn't get what is meant here.
There was a problem hiding this comment.
Thank you for the suggestion, have modified it to be clearer.
Co-authored-by: Victoriya Fedotova <viktoria.nn@gmail.com>
onedal/datatypes/_data_conversion.py
Outdated
| import dpnp | ||
|
|
||
| def _onedal_gpu_table_to_array(table, xp=None): |
There was a problem hiding this comment.
Why it is only gpu named? Will not these work with tables on CPU?
There was a problem hiding this comment.
Good call, changed to _table_to_array instead.
| #ifdef ONEDAL_DATA_PARALLEL | ||
| if (py::hasattr(obj, "__sycl_usm_array_interface__")) { | ||
| return convert_from_sua_iface(obj); | ||
| } | ||
| #endif // ONEDAL_DATA_PARALLEL |
| # investigate why dpnp.array(table, copy=False) doesn't work. | ||
| # Work around with using dpctl.tensor.asarray. | ||
| if xp == dpnp: | ||
| return dpnp.array(dpnp.dpctl.tensor.asarray(table), copy=False) |
There was a problem hiding this comment.
I have already asked this on #2126, but let me ask it here once again
I don't know is this dpctl module in dpnp is an official part of API? This may work now, but I am not sure for further versions of DPNP. It worth to add some comments here
There was a problem hiding this comment.
Good question https://github.com/IntelPython/dpnp/blob/master/dpnp/__init__.py#L38 It looks like they haven't figured out how to use os.add_dll_directory and are importing dpctl to get the file path and add it to the environment path. So as long as they need to find dpctl's shared objects, they will do it this way.
Co-authored-by: Samir Nasibli <samir.nasibli@intel.com>
Description
Changes needed to simplify its interface and to correct circular import problems occurring in #2126 (i.e. onedal.validation is used throughout, and adding onedal's assert_all_finite is not possible). There is an avoidance of using the dpep_helpers because of the circular import of importing
onedal.utilswhich imports some ofonedal.utils.validationin the__init__.pyfile, therefore importingonedal.utils._dpep_helpershas to be done carefully. A follow up PR with an associated ticket will revert this change in the future.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