Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AnderOne
Copy link

@AnderOne AnderOne commented Apr 5, 2019

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

Add checking for conversion of a right side to Array::T_numtype (C++11 is used)
@citibeth citibeth changed the title Fixing assignment between Array<TinyVector> and TinyVector expression (see blitzpp/blitz#108) Fixing assignment between Array<TinyVector> and TinyVector expression Apr 8, 2019
@citibeth
Copy link
Contributor

citibeth commented Apr 8, 2019

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
Copy link
Member

slayoo commented Apr 10, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants