add npy reader and writer #465
Conversation
|
@llohse we used your code. can we remove the MIT license and only keep the BSD, with your copyright? |
|
@wolfv: Yes, you may remove the MIT license. |
|
Note that the newest version of libnpy has compile-time type-checking and byte-order determination. |
|
ok, great, I modified the license in the header accordingly. Thanks for the great piece of work! Wolf |
|
hnggg... the github user interface offered me a merge of master ... probably we're gonna rebase anyways. |
|
indeed I think you'll want to rebase, squash, remove the merge commit and force push once the lights are green. |
|
@llohse awesome! By the way, we are working hard to support more and more features of numpy in xtensor (see the cheat sheet). Don't hesitate to hop on our gitter chat if you are interested. |
|
|
include/xtensor/xnpy.hpp
Outdated
| : m_shape(shape), m_fortran_order(fortran_order), m_typestring(typestring) | ||
| { | ||
| // Allocate memory | ||
| m_word_size = atoi(&typestring[2]); |
There was a problem hiding this comment.
Problem: The "typestring" can have a much more general form where the 3rd byte is not the width.
| size_t m_word_size; | ||
| size_t m_n_bytes; | ||
| std::string m_typestring; | ||
| char* m_buffer; |
There was a problem hiding this comment.
As discussed there is a memory leak here.
I see two options:
a) adding a boolean (pointer_moved, or has_been_cast) that indicates wether this object has been cast, and the pointer is owned by the xadaptor (which will take care of the destruction). If not, the destructor of npy_file will free the memory.
b) using a smart pointer (maybe unique, or shared) and hope that the xadaptor can handle them ;)
What do you think @SylvainCorlay?
There was a problem hiding this comment.
Maybe the npy file could have two actual C++ casting operators
-
one that return an
xarray_container<owning_buffer_adapter, L, std::vector<std::size_t>> &&which can only be called when the npy file is an rvalue. It takes ownership of the pointer and prevents the npy file destructor to actually delete the buffer with amoved_fromflag or setting the pointer in the class tonullptr. -
one to
xarray_adaptor<nonowning_buffer_adapter, L, std::vector<std::size_t>>which can only be called when the npy file is an lvalue. It does not take ownership of the pointer. When assigning it to a container, the generalized copy constructor is called and a copy occurs.
So basically
auto operator xarray_container<owning_buffer_adapter, L, std::vector<std::size_t>() &&
and
auto operator xarray_adaptor<const_nonowning_buffer_adapter, L, std::vector<std::size_t>() const &
and
auto operator xarray_adaptor<nonowning_buffer_adapter, L, std::vector<std::size_t>() &
include/xtensor/xnpy.hpp
Outdated
| if ((L == layout_type::column_major && !m_fortran_order) || | ||
| (L == layout_type::row_major && m_fortran_order)) | ||
| { | ||
| throw std::runtime_error("fortran order and xarray layout type do not match up."); |
There was a problem hiding this comment.
Suggestion: Layout mismatch between npy file and requested layout.
|
You can rebase on master now that the |
|
@SylvainCorlay if you could look at the casters ... this way it seems to somewhat work :) |
include/xtensor/xnpy.hpp
Outdated
| { | ||
| auto cast_elems = cast_impl<T, L>(check_type); | ||
| m_buffer = nullptr; | ||
| return std::move(xadapt(std::move(std::get<0>(cast_elems)), std::get<1>(cast_elems), |
There was a problem hiding this comment.
I think that there is no need to call std::move on xadapt since xadapt should already return an rvalue.
needs tests...