From f757aa487ad646aa4ed01a05c5352e79e95fc503 Mon Sep 17 00:00:00 2001
From: SOULIER Laurent <laurent.soulier@cea.fr>
Date: Wed, 6 Dec 2023 18:57:32 +0100
Subject: [PATCH] [fix/FlatId][FIX][DSGN] Redesigning flat index usage in
Tensor API, previous implementation was inconsistent fix #58 fix #60
---
include/aidge/backend/TensorImpl.hpp | 43 +++++++---------------------
include/aidge/data/Tensor.hpp | 42 ++++++++++++++++++++-------
src/data/Tensor.cpp | 34 ++++++++++++++++++++--
src/recipies/FuseBatchNorm.cpp | 2 ++
4 files changed, 75 insertions(+), 46 deletions(-)
diff --git a/include/aidge/backend/TensorImpl.hpp b/include/aidge/backend/TensorImpl.hpp
index 80460cdb7..7a3d33744 100644
--- a/include/aidge/backend/TensorImpl.hpp
+++ b/include/aidge/backend/TensorImpl.hpp
@@ -173,6 +173,7 @@ public:
/// is the tensor dimension.
/// @todo make it pure virtual, temporary default implementation provided for
/// testing and debugging
+ /// @todo May be deprecated as not use anywhere anymore
inline std::vector<std::size_t> const &getMemoryLayout() const noexcept
{
return mvLayout;
@@ -190,50 +191,23 @@ public:
return mDataType;
};
- /// @brief Get the logical coordinates of the data at given index
- /// @param flatIdx 1D index of the value considering a flatten tensor.
- /// @note The index is expressed in number of elements
- /// @return Logical coordinates of the data at given index
- /// @note return by value is efficient here as it relies on vector move semantic.
- std::vector<Coord_t> getCoord(NbElts_t const flatIdx) const noexcept
- {
- std::vector<Coord_t> coordIdx(mvDimensions.size());
- std::size_t In = flatIdx;
- // trick for reverse loop without index underflow
- // when i is 1, (i--)>0 is true but i contains 0 on ouput
- // when i is 0, (i--)>0 is false leaving the loop, i has underflow but its
- // value is not used anymore
- /// @todo implement it with synchronized reverse iterators
- for (std::size_t i = mvDimensions.size(); (i--) > 0;)
- {
- // computes coordinates in 0-based coordinates
- coordIdx[i] = In % mvDimensions[i];
- In = (In - coordIdx[i]) / mvDimensions[i];
- // shift with respect to first data coordinates
- coordIdx[i] = coordIdx[i] + mvFirstDataCoordinates[i];
- }
-
- return coordIdx;
- };
-
- /// @brief Get the linear index of the first Byte_t of the data at given
- /// coordinates
+ /// @brief Get the linear index of the data at given coordinates
/// @param coordIdx coordinates of the desired data
/// @note The index is expressed in number of elements
- /// @return Linear index of the first Byte_t of the data at given coordinates
- NbElts_t getIdx(std::vector<Coord_t> const &coordIdx) const noexcept
+ /// @return Linear index of the the data at given coordinates
+ NbElts_t getImplIdx(std::vector<Coord_t> const &coordIdx) const noexcept
{
std::size_t flatIdx(0);
- for (std::size_t i = 0; i < mvDimensions.size(); ++i)
+ for (std::size_t i = 0; i < mvDimensions.size()-1; ++i)
{
assert(
((coordIdx[i] - mvFirstDataCoordinates[i]) < mvDimensions[i])
&& (coordIdx[i] >= mvFirstDataCoordinates[i])
&& "Coordinates dimensions does not fit the dimensions of the "
"tensor");
- flatIdx += ((coordIdx[i] - mvFirstDataCoordinates[i]) * mvLayout[i]);
+ flatIdx = (flatIdx+(coordIdx[i] - mvFirstDataCoordinates[i])) * mvDimensions[i+1];
}
- return flatIdx / mScalarSize;
+ return flatIdx+(coordIdx[mvDimensions.size()-1] - mvFirstDataCoordinates[mvDimensions.size()-1]);
}
/// @brief Change TensorImpl dimensions
@@ -278,6 +252,7 @@ private:
/// mDimensions[D-1], then mvLayout[D-1] is mScalarSize and
/// mvLayout[i] == mvLayout[i+1]*mDimensions[i+1];<br>
/// Besides mvLayout[0]*mDimensions[0] == mNbElts*mvLayout[D-1].
+ /// @todo May be deprecated as not use anywhere anymore
std::vector<std::size_t> mvLayout;
/// @brief Address of the very first data in memory (lexicographic order)
/// @details Points to a contiguous in-memory array such that, when cast to
@@ -366,6 +341,7 @@ protected:
/// @brief Computes layout from Tensor total dimensions and size of a single data.
/// @note Assumes that the Tensor dimensions and data type are valid.
/// @note Should be used only during (re)initialization of storage.
+ /// @todo May be deprecated as not use anywhere anymore
void computeLayout()
{
std::size_t NbDims = mvDimensions.size();
@@ -416,6 +392,7 @@ protected:
clonePropertiesExceptArea(i_TensorImpl);
mvDimensions = i_TensorImpl.mvDimensions;
mvFirstDataCoordinates = i_TensorImpl.mvFirstDataCoordinates;
+ /// @todo May be deprecated as not use anywhere anymore
mvLayout = i_TensorImpl.mvLayout;
}
/// Â @brief Release internal storage
diff --git a/include/aidge/data/Tensor.hpp b/include/aidge/data/Tensor.hpp
index 2dc478519..a6c954a6b 100644
--- a/include/aidge/data/Tensor.hpp
+++ b/include/aidge/data/Tensor.hpp
@@ -463,45 +463,66 @@ public:
return mDims.empty();
}
- /// @brief Get element by its index
+ /// @brief Get element by its index within the active area
/// @tparam expectedType expected type of the stored element
- /// @param idx offset in number of elements from the first stored element
+ /// @param idx index, in number of elements and lexical order, from the first data within active area
/// @return read-write reference to the element
/// @bug this function is plain wrong and can be used only on cpu backend
/// @todo redesign
+ /// @todo change API idx to become a logical index inside active area only
template<typename expectedType> expectedType &get(NbElts_t const idx) noexcept
{
///@todo : add assert expected Type compatible with datatype
///@todo : add assert idx < Size
+ // converts logical flat id to storage flat id
return *reinterpret_cast<expectedType *>(
- getDataAddress() + idx * getScalarSize());
+ getDataAddress() + mImpl->getImplIdx(getCoord(idx)) * getScalarSize());
}
+ /// @brief Get element by its logical coordinates
+ /// @tparam expectedType expected type of the stored element
+ /// @param coordIdx coordinates of the desired data
+ /// @return read-write reference to the data
+ /// @bug this function is plain wrong and can be used only on cpu backend
+ /// @todo redesign
+ /// @todo change API idx to become a logical index inside active area only
template<typename expectedType>
expectedType &get(std::vector<Coord_t> const &coordIdx)
{
- return get<expectedType>(getIdx(coordIdx));
+ ///@todo : add assert expected Type compatible with datatype
+ ///@todo : add assert coordIdx inside active area
+ return *reinterpret_cast<expectedType *>(
+ getDataAddress() + mImpl->getImplIdx(coordIdx) * getScalarSize());
}
- /// @brief Set teh value of an element identified by its index
+ /// @brief Set the value of an element identified by its index within active area
/// @tparam expectedType expected type of the stored element
- /// @param idx offset in number of elements from the first stored element
- /// @return read-write reference to the element
+ /// @param idx index, in number of elements and lexical order, from the first data within active area
+ /// @param value value to set at given index
/// @bug this function is plain wrong and can be used only on cpu backend
/// @todo redesign
template<typename expectedType> void set(NbElts_t const idx, expectedType const value)
{
///@todo : add assert expected Type compatible with datatype
///@todo : add assert idx < Size
- unsigned char *dataPtr = getDataAddress() + idx * getScalarSize();
+ // converts logical flat id to storage flat id
+ unsigned char *dataPtr = getDataAddress() + mImpl->getImplIdx(getCoord(idx)) * getScalarSize();
///@bug only valid for trivially copyable data
std::memcpy(dataPtr, &value, getScalarSize());
}
+ /// @brief Set the value of a data identified by its logical coordinates
+ /// @tparam expectedType expected type of the stored data
+ /// @param coordIdx logical coordinates of the data to set
+ /// @param value value to set at given coordinates
template<typename expectedType>
void set(std::vector<Coord_t> coordIdx, expectedType value)
{
- set<expectedType>(getIdx(coordIdx), value);
+ ///@todo : add assert expected Type compatible with datatype
+ ///@todo : add assert coordIdx inside active area
+ unsigned char *dataPtr = getDataAddress() + mImpl->getImplIdx(coordIdx) * getScalarSize();
+ ///@bug only valid for trivially copyable data
+ std::memcpy(dataPtr, &value, getScalarSize());
}
std::string toString() const;
@@ -517,7 +538,7 @@ public:
* @brief From the the 1D index, return the coordinate of an element in the tensor.
*
* @param flatIdx 1D index of the value considering a flatten tensor.
- * @return std::vector<DimSize_t>
+ * @return std::vector<Coord_t>
* @note return by value is efficient here as it relies on vector move semantic.
*/
std::vector<Coord_t> getCoord(NbElts_t const flatIdx) const noexcept;
@@ -531,6 +552,7 @@ public:
NbElts_t getIdx(std::vector<Coord_t> const &coordIdx) const noexcept;
private:
+
/// @brief Getting the address of the very first data in memory (lexicographic
/// order), read only access to data
/// @details Points to a contiguous in-memory array such that, when cast to
diff --git a/src/data/Tensor.cpp b/src/data/Tensor.cpp
index 973712881..476ef1e50 100644
--- a/src/data/Tensor.cpp
+++ b/src/data/Tensor.cpp
@@ -11,6 +11,8 @@
#include "aidge/data/Tensor.hpp"
+#include <cassert>
+
namespace Aidge
{
/// @brief Assess data type, dimensions, backend and data are the same.
@@ -64,12 +66,38 @@ void Tensor::copyFromHost(Byte_t const *const srcPtr, std::size_t const Bytes)
std::vector<Coord_t> Tensor::getCoord(NbElts_t const flatIdx) const noexcept
{
- return mImpl->getCoord(flatIdx);
-}
+ std::vector<Coord_t> coordIdx(mDims.size());
+ std::size_t Index = flatIdx;
+ // trick for reverse loop without index underflow
+ // when i is 1, (i--)>0 is true but i contains 0 on ouput
+ // when i is 0, (i--)>0 is false leaving the loop, i has underflow but its
+ // value is not used anymore
+ /// @todo implement it with synchronized reverse iterators
+ for (std::size_t i = mDims.size(); (i--) > 0;)
+ {
+ // computes coordinates in 0-based coordinates
+ coordIdx[i] = Index % mDims[i];
+ Index = (Index - coordIdx[i]) / mDims[i];
+ // shift with respect to first data coordinates
+ coordIdx[i] = coordIdx[i] + mvActiveAreaOrigin[i];
+ }
+
+ return coordIdx;
+};
NbElts_t Tensor::getIdx(std::vector<Coord_t> const &coordIdx) const noexcept
{
- return mImpl->getIdx(coordIdx);
+ std::size_t flatIdx(0);
+ for (std::size_t i = 0; i < mDims.size()-1; ++i)
+ {
+ assert(
+ ((coordIdx[i] - mvActiveAreaOrigin[i]) < mDims[i])
+ && (coordIdx[i] >= mvActiveAreaOrigin[i])
+ && "Coordinates dimensions does not fit the dimensions of the "
+ "tensor");
+ flatIdx = (flatIdx+(coordIdx[i] - mvActiveAreaOrigin[i])) * mDims[i+1];
+ }
+ return flatIdx+(coordIdx[mDims.size()-1] - mvActiveAreaOrigin[mDims.size()-1]);
}
/// @brief Getting the address of the very first data in memory (lexicographic
diff --git a/src/recipies/FuseBatchNorm.cpp b/src/recipies/FuseBatchNorm.cpp
index 549e11eae..dd4a2ad86 100644
--- a/src/recipies/FuseBatchNorm.cpp
+++ b/src/recipies/FuseBatchNorm.cpp
@@ -110,6 +110,8 @@ void Aidge::fuseBatchNorm(
}
}
+ /// @todo FIXME
+ /// Following get set are possibly wrong as in memory index might be different from logical index
// TODO : check if noBias==true is set, then set biasValue to 0
float biasValue = bias->get<float>(outChId);
--
GitLab