Hi Navneet,
Navneet Dalal wrote:
> Hi Julian,
>
>>> 3. In TinyVector class (tinyvec.h and tinyvec.cc), all constructors
>>> take T_numtype argument, a slightly better way may be to pass const
>>> T_numtype&: for e.g. inline TinyVector(const T_numtype&
>>> initValue); compare to
>>> inline TinyVector(T_numtype initValue);
>>>
>>> The same holds for all variants of TinyVector(T_numtype ...
>>> constructors.
>>
>>
>> This is mostly a matter of programming style. I typically pass
>> arguments by value if
>> the objects are small simple types such as the built-in number types.
>> That is how it
>> happens to be done here in blitz. Also, tvmet constructs its Vector
>> with pass-by-value.
>> There is only an efficiency issue if the object is expensive to copy.
>
>
> I donot agree that it is just a matter of programming style. As you
> agree that there is an efficiency bottleneck if the TinyVector's are
> not constructed for built-in types.
>
> One may use it for any kind of Objects. It would be naive to assume
> that TinyVector should
> only be used for built in types. Since efficiency is one of the prime
> goal of blitz, I am unable to comprehend why these changes should not
> be made.
Realistically, the TinyVector is meant to hold numerical types with
which one can perform mathematical operations. That is the sole purpose
of this class. Even user-defined number types should provide a trivial
copy constructor or use a compiler-generated one. This has to be an
inexpensive operation or else the class would be useless. In addition,
TinyVector is itself a concrete data type, meaning it contains its own
data by value and does not allocate any pointers. So the constructor
has to make a copy of the arguments passed in and store them anyway.
You cannot avoid copying the values into local storage inside the
TinyVector.
The main reason not to pass by const reference here is that for the most
typical case where the argument type is a built-in numerical type, it
will be *slower* than passing by value because of the work required to
access the referenced memory. It doesn't make sense to make the most
common case less efficient, so that rare cases might go faster. If
others here on the list have a different take on this, please chime in,
but that is my understanding of this issue and why the code is written
the way it is.
>
>>> 4. in file array_impl.h, isInRange should be changed to
>>>
>>> template<class T>
>>> _bz_bool isInRange(const TinyVector<T,N_rank>& index) const
>>> // Was : _bz_bool isInRange(const T_index& index) const
>>>
>>>
>>> This will allow to check range for TinyVector values of types double.
>>> T_index is typedef to TinyVector<int, rank>.
>>>
>> The reason why this is limited to TinyVectors of ints is that blitz
>> Arrays can only
>> be indexed with int arguments. Indexing an Array with floating-point
>> indices is
>> not defined. Thus, there is really no purpose to checking whether a
>> TinyVector
>> of doubles falls within the index domain of an Array (at least, none
>> that I can think of).
>> Of course, if you *really* need to do this, you can loop over the
>> elements of your
>> TinyVector and check that each value lies between base(d) and base(d)
>> +length(d).
>
>
> I use blitz in image processing. Frequently, I need to check if a
> point (of double)
> is in domain of image-space (i.e. blitz array). So here is the need.
>
> Ofcourse, one can loop over the elements, but if one looks isInrange
> function code, then this
> is exactly what is written. My policy in this manner is, if one can
> avoid rewriting the same code again and again, one should avoid (esp
> if it is already present in the library).
> And further, I donot see any problems with this change. It is a small
> extension without any compromise on efficiency.
>
> In fact, I feel that all overloaded isInRange functions should accept
> template arguments.
>
I agree that this change would not harm efficiency. But if you look at
the code comments, you will see that these isInRange() functions are
meant to be internal debugging functions. They are used solely to check
Array indexing arguments before an indexing operation is done. These
checks are performed only when Blitz is compiled in debug mode. The
isInRange() functions are not listed in the Blitz library documentation,
and I believe they were never intended for end users. (Todd, am I wrong
here?) Thus, if anything, these functions should be made "private" to
the Array class. I am loathe to extend parts of Blitz that aren't part
of the official interface, especially when the existing interface
already provides a means of performing a task. Nothing is stopping you
from writing the function that takes a Blitz Array and a TinyVector of
doubles and tests whether the point is within the domain of the Array.
I don't think this needs to be part of the Array interface.
Again, if others think I am crazy and this change is critical, please
chime in.
Regards, Julian C.
--
Dr. Julian C. Cummings E-mail: cummings@...
California Institute of Technology Phone: 626-395-2543
1200 E. California Blvd., Mail Code 158-79 Fax: 626-584-5917
Pasadena, CA 91125
|