search

blitzpp / blitz

Blitz++ Multi-Dimensional Array Library for C++
https://github.com/blitzpp/blitz/wiki
Other
404 stars 83 forks source link

Fixing assignment between Array<TinyVector> and TinyVector expression #109

Open AnderOne opened 5 years ago

AnderOne commented 5 years ago

See #108 These changes require C++11 support!

citibeth commented 5 years ago

I have mixed feelings about this PR.

My main reservation is a concern of Blitz++ as a project without direction; including lack of agreement on its fundamental purpose. A LOT has changed since Blitz++ was first built. To me at least, the fundamental purpose of Blitz++ is to provide a rough equivalent to Fortran 90 arrays / numpy.array for C++. However, this was clearly not the original intent of the package; which also provides features for convolution, for example. In today's C++ environment, given a mythical clean slate, I think a library that just does multi-dimensional arrays would be best. That is what the (mythical vaporware) Blitz11 project is all about.

I personally have only used blitz::Array<double,...> and blitz::Array<int,...>, etc. I had not thought of nesting a TinyVector or TinyMatrix inside blitz::Array; and I'd seen TinyVector as basically a way to deal with dimensions, indices, etc. But it's clear that Blitz++ did intend this use, see the docs:

http://dsec.pku.edu.cn/~mendl/blitz/manual/blitz02.html

2.1.2: Array types The Array<T,N> class supports a variety of arrays:

Arrays of scalar types, such as Array<int,1> and Array<float,3> Complex arrays, such as Array<complex,2> Arrays of user-defined types. If you have a class called Polynomial, then Array<Polynomial,2> is an array of Polynomial objects.

Nested homogeneous arrays using TinyVector and TinyMatrix, in which each element is a fixed-size vector or array. For example, Array<TinyVector<float,3>,3> is a three-dimensional vector field.

Nested heterogeneous arrays, such as Array<Array<int,1>,1>, in which each element is a variable-length array.

For this reason, I would classify this PR as a bug fix on an originally intended feature; so clearly that's a good thing. And I don't mind requiring C++11 at this point. On the other hand, the number of lines involved in a "minor" bug fix makes me wonder how much we should be investing in this codebase; or what other large-scale approaches to minor bugs we might need in the future.

slayoo commented 5 years ago

Let me just +1 "don't mind requireing C++11"