-
Notifications
You must be signed in to change notification settings - Fork 82
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
base: main
Are you sure you want to change the base?
Conversation
Add checking for conversion of a right side to Array::T_numtype (C++11 is used)
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
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. |
Let me just +1 "don't mind requireing C++11" |
See #108
These changes require C++11 support!