-
Notifications
You must be signed in to change notification settings - Fork 21
Implement dpnp.interp()
#2417
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
base: master
Are you sure you want to change the base?
Implement dpnp.interp()
#2417
Conversation
Array API standard conformance tests for dpnp=0.18.0dev1=py312he4f9c94_56 ran successfully. |
View rendered docs @ https://intelpython.github.io/dpnp/pull/2417/index.html |
dpnp/backend/extensions/ufunc/elementwise_functions/interpolate.cpp
Outdated
Show resolved
Hide resolved
dpnp/backend/extensions/ufunc/elementwise_functions/interpolate.cpp
Outdated
Show resolved
Hide resolved
dpnp/backend/extensions/ufunc/elementwise_functions/interpolate.cpp
Outdated
Show resolved
Hide resolved
dpnp/backend/extensions/ufunc/elementwise_functions/interpolate.cpp
Outdated
Show resolved
Hide resolved
x = dpnp.asarray(x, dtype=x_float_type, order="C") | ||
xp = dpnp.asarray(xp, dtype=x_float_type, order="C") | ||
|
||
out_dtype = dpnp.common_type(x, xp, fp) |
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 guess the result has at least default real floating data type, isn't it?
out_dtype = dpnp.common_type(x, xp, fp) | |
out_dtype = dpnp.common_type(x_float_type, fp) |
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.
dpnp.common_type
takes *arrays as input argument and I can not pass x_float_type
|
||
if period is not None: | ||
period = _validate_interp_param(period, "period", exec_q, usm_type) | ||
if period == 0: |
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 check leads to SYCL queue synchronization. Is that expected?
struct value_type_of<std::complex<T>> | ||
{ | ||
using type = T; | ||
}; |
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.
This implementation has a problem of not supporting const std::complex<T>
case.
Better approach could be something like:
template<typename T, bool hasValueType>
struct value_type_of_impl;
template<typename T>
struct value_type_of_impl<T, false>
{
using type = T;
};
template<typename T>
struct value_type_of_impl<T, true>
{
using type = typename T::value_type;
};
template<T>
using value_type_of = value_type_of_impl<T, is_complex_v<T>>;
template<T>
using value_type_of_t = typename value_type_of<T>::type;
And in future is_complex
could be replaced with proper has_value_type
if needed.
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 is good point but I would prefer using type = typename std::remove_cv_t<T>::value_type
to ensure it works correctly with const
types
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.
Do you mean something like?
struct value_type_of<std::complex<T>>
{
using type = typename std::remove_cv_t<T>::value_type;
};
Then, I believe, it will not work.
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 actually meant removing const
in value_type_of_impl<T, true>
template<typename T>
struct value_type_of_impl<T, true>
{
using type = typename std::remove_cv_t<T>::value_type;
};
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 in 9e06cc3
This PR suggests adding an implementation of
dpnp.interp()
with sycl_kernel including support for all function arguments.