Conversation
fb73857 to
02a723d
Compare
include/zarray/zarray.hpp
Outdated
| const nlohmann::json& get_metadata() const; | ||
| void set_metadata(const nlohmann::json& metadata); | ||
|
|
||
| zarray_impl* make_view(xstrided_slice_vector& slices); |
There was a problem hiding this comment.
THis method should return a zarray object wrapping the zarray_impl* returned by the make_view method of zarray_impl
include/zarray/zarray_impl.hpp
Outdated
| inline zarray_impl* zexpression_wrapper<CTE>::make_view(xstrided_slice_vector& slices) | ||
| { | ||
| auto e = strided_view(m_expression, slices); | ||
| zarray_impl* p_impl = e.derived_cast().allocate_result(); |
There was a problem hiding this comment.
e.derived_cast() is an xexpression that do not provide the allocate_result method. What you want to do here is build a zexpression_wrapper on the newly created strided_view:
return build_zarray(std::move(e));
include/zarray/zarray_impl.hpp
Outdated
| inline zarray_impl* zscalar_wrapper<CTE>::make_view(xstrided_slice_vector& slices) | ||
| { | ||
| auto e = strided_view(m_array, slices); | ||
| zarray_impl* p_impl = e.derived_cast().allocate_result(); |
include/zarray/zarray_impl.hpp
Outdated
| inline zarray_impl* zarray_wrapper<CTE>::make_view(xstrided_slice_vector& slices) | ||
| { | ||
| auto e = strided_view(m_array, slices); | ||
| zarray_impl* p_impl = e.derived_cast().allocate_result(); |
include/zarray/zarray_impl.hpp
Outdated
| inline zarray_impl* zchunked_wrapper<CTE>::make_view(xstrided_slice_vector& slices) | ||
| { | ||
| auto e = strided_view(m_chunked_array, slices); | ||
| zarray_impl* p_impl = e.derived_cast().allocate_result(); |
82baf88 to
0a049f9
Compare
|
I couldn't use |
JohanMabille
left a comment
There was a problem hiding this comment.
I think the file zview.hpp and the zview_dispatcher can be removed.
include/zarray/zarray_impl.hpp
Outdated
| namespace detail | ||
| { | ||
| template <class E> | ||
| struct is_a_strided_view : std::false_type |
There was a problem hiding this comment.
Nitpicking: could be is_xstrided_view to be consistent with is_xarray just below
include/zarray/zarray_impl.hpp
Outdated
| { | ||
| }; | ||
|
|
||
| template <class E, class S> |
There was a problem hiding this comment.
I think you could add the additional template arguments of xstrided_view to be sure we can match all the cases.
include/zarray/zarray_impl.hpp
Outdated
| { | ||
| compute_cache(); | ||
| return m_cache.get_offset(); | ||
| auto e = xt::strided_view(m_cache, slices); |
There was a problem hiding this comment.
What about:
auto e = xt::strided_view(get_array(), slices);This way you're not exposed to the caching mechanism detail.
|
Great! I'll merge when the CI is green. |
No description provided.