[vsipl++] Matlab IO

[Assem Salama]

Jules Bergmann wrote:
> Assem, I have 8 comments below.  -- Jules
>
> > +  template <vsip::dimension_type Dim>
> > +  struct view_header
> > +  {
> > +    data_element header;
> > +    data_element array_flags_header;
> > +    uint32_t array_flags[2];
> > +    data_element dim_header;
> > +    uint32_t dim[Dim + Dim%2]; //the dim has to be aligned to an 8 
> byte boundary
>
> [1] Can you make sure this comment fits into 80-columns?
Yes it is.
>
> > +    data_element array_name_header;
> > +  };
>
> > +  // the read function for real or complex depending of the view 
> that was
> > +  // passed in
> > +  template <typename T1,
> > +        typename ViewT>
> > +  void read(std::istream& is,ViewT v,bool swap_bytes)
> > +  {
> > +    vsip::dimension_type const View_dim = ViewT::dim;
> > +    vsip::Index<View_dim> my_index;
> > +    vsip::impl::Length<View_dim> v_extent = extent(v);
> > +    T1 data;
> > +    typedef typename ViewT::value_type scalar_type;
> > +
> > +    // get num_points
> > +    vsip::length_type num_points = v.size();
> > +
> > +    // read all the points
> > +    for(vsip::index_type i=0;i<num_points;i++) {
> > +      is.read(reinterpret_cast<char*>(&data),sizeof(data));
> > +      swap<T1>(&data,swap_bytes);
>
> [2] The intent of the wrapper template function 'swap' is to let the
> compiler infer template parameters automatically.  Since 'data' is of
> type 'T1', you can just say:
>
>     swap(&data, swap_bytes);
>
> Unless the inferred type would be incorrect, can you remove the 
> explicit parameter?
>
>
> > +
> > +// operator to read view from matlab file
> > +template <typename T,
> > +          typename Block0,
> > +      template <typename,typename> class View>
> > +inline
> > +std::istream&
> > +operator>>(
> > +  std::istream&                                       is,
> > +  Matlab_bin_formatter<View<T,Block0> >               mbf)
> > +{
> > +  matlab::data_element temp_data_element;
> > +  matlab::view_header<vsip::impl::Dim_of_view<View>::dim> m_view;
> > +  typedef typename vsip::impl::Scalar_of<T>::type scalar_type;
> > +  typedef matlab::Subview_helper<View<T,Block0> > subview;
> > +  typedef typename subview::realview_type r_v;
> > +  typedef typename subview::imagview_type i_v;
> > +  vsip::dimension_type v_dim = vsip::impl::Dim_of_view<View>::dim;
> > +
> > +
> > +  // read header
> > +  is.read(reinterpret_cast<char*>(&m_view),sizeof(m_view));
> > +
>
> [3] When does mbf.header.endian get initialized?  Have you written a
> unit test for these routines?  (If not, please do that next).
>
mbf.header gets initialized in the constructor. I have a test that 
writes and read vectors,matrices,and tensors.
>
> > +  // do we need to swap fields?
> > +  matlab::swap_header(m_view,mbf.header.endian);
>
> [4] The second arg to swap_header is a bool indicating if bytes should
> be swapped.  Passing mbf.header.endian (a uint16_t) will nearly always
> be true (unless the matlab file is corrupted).  Can you do something
> like:
>
>     bool swap_bytes;
>
>     if      (mbf.header.endian == 'M' << 8 | 'I') swap_bytes = false;
>     else if (mbf.header.endian == 'I' << 8 | 'M') swap_bytes = true;
>     else
>       VSIP_IMPL_THROW(std::runtime_error(
>         "Matlab file header has invalid endian field"));
>
>     matlab::swap_header(m_view, swap_bytes);
>
>
> > +
> > +  // is this complex?
> > +  if(vsip::impl::Is_complex<T>::value && 
> !(m_view.array_flags[0]&(1<<11)))
> > +    VSIP_IMPL_THROW(std::runtime_error(
> > +      "Trying to read complex matrix into a real matrix"));
>
> [5] Please change this to "Trying to read a complex view into a real 
> view"
>
> > +
> > +
> > +  // is this the same class?
> > +  if(!((m_view.array_flags[0] & 0xff) ==
> > +            (matlab::Matlab_header_traits<sizeof(scalar_type),
> > +                  std::numeric_limits<scalar_type>::is_signed,
> > + std::numeric_limits<scalar_type>::is_integer>::class_type)
> > +        ))
> > +    VSIP_IMPL_THROW(std::runtime_error(
> > +      "Trying to read a matrix of a different class"));
>
> [6] Please change this to "Trying to read a view with different class 
> of value type"
>
> > +
> > +  // do dimensions agree?
> > +  if(v_dim == 1) m_view.dim_header.size -= 4; // special case for 
> vectors
> > +  if(v_dim != (m_view.dim_header.size/4))
> > +    VSIP_IMPL_THROW(std::runtime_error(
> > +      "Trying to read a matrix of different dimensions"));
>
> [7] Please change this to "Trying to read a view with different 
> dimension"
>
> > +
> > +  for(vsip::dimension_type i=0;i<v_dim;i++)
> > +    if(mbf.view.size(i) != m_view.dim[i])
> > +      VSIP_IMPL_THROW(std::runtime_error(
> > +        "Matrix dimensions don't agree"));
>
> [8] Please change this to "View dimensions don't agree"
>
>