Add LinearAlgebra::Vector derived from ReadWriteVector and VectorSpaceVector #1800
Conversation
|
|
||
| /** | ||
| * Initialize the vector with a given range of values pointed to by the | ||
| * iterators. This function is there in analogy to the @p std::vector class. |
There was a problem hiding this comment.
This function is there -> This function exists
|
I don't quite remember our conclusion on how the final layout of the classes is going to look like. Is this the replacement for the current Did we decide that it is supposed to be a ReadWriteVector and a VectorSpaceVector? One argument against this is that serial and parallel code will look quite different. |
| @@ -154,7 +157,7 @@ namespace LinearAlgebra | |||
| virtual size_type size() const = 0; | |||
|
|
|||
| /** | |||
| * Return an index set that describes which elements of this vector are | |||
| * Returns an index set that describes which elements of this vector are | |||
There was a problem hiding this comment.
We're inconsistent in the way we use this, but I think in most places we use "Return this and that" as opposed to "[This function] Returns this and that."
|
On 10/27/2015 03:54 PM, Timo Heister wrote:
In the longer term yes.
Correct. At least if they need element-wise access. It will no longer be
Yes, I think we agreed as a group (I know you didn't) that it's ok to |
| typename ReadWriteVector<Number>::real_type norm = 0.; | ||
| for (unsigned int i=0; i<this->size(); ++i) | ||
| if (std::abs(this->val[i])>norm) | ||
| norm = std::abs(this->val[i]); |
|
I have done the modifications. |
… and LinearAlgebra::VectorSpaceVector.
|
I have added read/write functions and |
|
Can we merge this. |
|
I looked over the changes again and it looks good to me, but I would prefer a few more eyes on this before we merge it. It is a complicated patch after all. @kronbichler ? |
| /** | ||
| * Scaling and simple vector addition, i.e. <tt>*this = s*(*this)+V</tt>. | ||
| */ | ||
| virtual void sadd(const Number s, const VectorSpaceVector<Number> &V); |
There was a problem hiding this comment.
This is a more general question: Any reason to have this method? One can easily obtain the desired behavior by sadd(s, 1.0, V). Since we're changing interfaces right now anyway, why not minimize here?
There was a problem hiding this comment.
Can't this be optimized further than sadd(s,1.0,V)? Otherwise, we could get ride of it.
There was a problem hiding this comment.
If it's redundant and doesn't save a lot of CPU time, I'm in favor of getting rid of things.
There was a problem hiding this comment.
(To be sure, I would imagine that an operation such as sadd(s,1.0,V) is entirely memory-bound, and so no slower than sadd(s,V) which simply omits the multiplication.)
There was a problem hiding this comment.
I agree with @bangerth . The redundant multiplication by 1.0 will definitely not show up on any reasonable processor. Not even if the vectors were in L1 cache and the operation was fully vectorized.
|
A more general remark is why this vector is not using any parallelism, i.e., all the initialization and computation is done with a single thread. Is this a temporary solution (i.e., we will later import all the 'optimized' functions from dealii::Vector) or is this on purpose and will there be a separate vector class for thread parallelism? As for an initial realization and establishing the interfaces, I have no further comments apart from the above two questions. Regarding implementation I would take over the implementations from dealii::Vector (the parallelism in vector manipulations, reductions, initialization (first-touch), memory access optimizations...) |
|
Yes, the implementation will change in the future (same for |
Agreed. |
|
So I suggest you remove the |
Add LinearAlgebra::Vector derived from ReadWriteVector and VectorSpaceVector
| * C++ standard library, this class implements an element of a vector space | ||
| * suitable for numerical computations. | ||
| * | ||
| * @authod Bruno Turcksin, 2015. |
This is the first vector class based on the new interface when it will be done it should replace the current Vector.