- Notifications
You must be signed in to change notification settings - Fork64
Adding possibility to 'cast' or copy toxt::xarray etc#267
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xt::xarray etcThere 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'm not sure it would make sense to implement the cast operator forstrided_views (which is used for creating a view on an existing expression).
It would make sense to implement it for thexarray_adaptor, though. It would be a non-owning adpator on the underlying buffer.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| template <class T> | ||
| struct xtensor_check_buffer | ||
| { | ||
| }; |
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 you can omit the definition of the generic case. A simple declaratoin should be enough.
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 thought so too, but my compiler complained:
xtensor_type_caster_base.hpp:50:16: error: notemplatenamed'xtensor_check_buffer'; did you mean'xtensor_get_buffer'?structxtensor_check_buffer<xt::xarray<T, L>> ^~~~~~~~~~~~~~~~~~~~ xtensor_get_buffer
| std::vector<size_t> shape(buf.ndim()); | ||
| std::copy(buf.shape(), buf.shape() + buf.ndim(), shape.begin()); | ||
| value = Type(shape); | ||
| std::copy(buf.data(), buf.data() + buf.size(), value.begin()); |
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 take into consideration the strides of the numpy array. There is no guarantee that the element of the arrayare contiguous in memory (the array could actually be a view).
The strides number are numbers of bytes instead of numbers of elements, so we have to handle the conversion. Or we can use thepystride adaptor class.
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.
Ah, I did not know about the pystride adaptor. So for the moment I use pybind11's way to ensure that data is contiguous, by usinghttps://pybind11.readthedocs.io/en/stable/advanced/pycpp/numpy.html#arrays. Personally I have no big feelings about one or the other, but at the same time I am not entirely sure about the difference in efficiency.
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.
Ah indeedc_style andf_style involve contiguity, I never paid attention to that, thanks for the pointer!
tdegeus commentedJun 5, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Essentially this should be good to go. However, I had to introduce on additional check to check on rank for staticautoget(pybind11::handle src){auto buf = pybind11::array_t<T, pybind11::array::c_style | pybind11::array::forcecast>::ensure(src);if (buf.ndim() !=2) {returnfalse; }return buf;} however my compiler gets (rightfully) confused: So I worked around with the extra check. I would like to get rid of it. But I don't know what I can return instead of if (!buf) {returnfalse;} Alternatively on could try to get rid of the check statically. But this I don't know how to do easily. |
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.
Looks good to me once the conflicts are solved. I've suggested some renaming that I find more explicit.
| { | ||
| return array_t<T, array::c_style | array::forcecast>::ensure(src); | ||
| } | ||
| }; |
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.
Nitpicking: what about the following renaming?xtensor_get_buffer =>pybind_array_getter_implget =>run
| { | ||
| return xtensor_get_buffer<T, L>::get(src); | ||
| } | ||
| }; |
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.
Nitpicking: in line with the previous renaming proposal:xtensor_chek_buffer =>pybind_array_getterget =>run
| { | ||
| return true; | ||
| } | ||
| }; |
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.
Nitpicking:xtensor_verify =>pybind_array_dim_checkerget =>run
| { | ||
| using T = typename Type::value_type; | ||
| if (!convert && !array_t<T>::check_(src)) { |
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.
Nitpicking: opening curly brace goes on a new line.
Thanks@JohanMabille ! Good to go. |
No description provided.