Expand ListOfLists pushBack() functionality
authorBerk Hess <hess@kth.se>
Sun, 15 Dec 2019 21:45:48 +0000 (22:45 +0100)
committerBerk Hess <hess@kth.se>
Thu, 19 Dec 2019 12:56:28 +0000 (13:56 +0100)
Added member pushBackListOfSize(int) to ListOfLists to add a list of a
given size to the list so its elements can be set. Used this in
applicable cases and added back() to ListOfLists to facilitate access
to that newly added list. Added front() for symmetry. Added tests.

Limited ListOfLists to arithmetic types, as there is no compelling
use case for other types and it simplifies the class.

Also fixed two minor issues that came up during review.

Change-Id: Ia05c270f9c337902c78f97e9613e45e82a21df63

src/gromacs/domdec/domdec_topology.cpp
src/gromacs/gmxpreprocess/gpp_nextnb.cpp
src/gromacs/nbnxm/benchmark/bench_system.cpp
src/gromacs/selection/tests/nbsearch.cpp
src/gromacs/topology/tests/exclusionblocks.cpp
src/gromacs/utility/listoflists.h
src/gromacs/utility/tests/listoflists.cpp

index 23346db87cdf6f220e4469510edd57250ae5c537..502ae91eedc9f80ca05ce4657a76b7e385278c8c 100644 (file)
@@ -993,15 +993,6 @@ static real dd_dist2(t_pbc* pbc_null, const rvec* x, const int i, int j)
     return norm2(dx);
 }
 
-/*! \brief Append ListOfLists exclusion objects 1 to nsrc in \p src to \p *dest */
-static void combineExclusions(ListOfLists<int>* dest, gmx::ArrayRef<const thread_work_t> src)
-{
-    for (gmx::index s = 1; s < src.ssize(); s++)
-    {
-        dest->appendListOfLists(src[s].excl);
-    }
-}
-
 /*! \brief Append t_idef structures 1 to nsrc in src to *dest */
 static void combine_idef(t_idef* dest, gmx::ArrayRef<const thread_work_t> src)
 {
@@ -1580,11 +1571,10 @@ static int make_local_bondeds_excls(gmx_domdec_t*       dd,
 
         if (izone < nzone_excl)
         {
-            if (rt->th_work.size() > 1)
+            for (std::size_t th = 1; th < rt->th_work.size(); th++)
             {
-                combineExclusions(lexcls, rt->th_work);
+                lexcls->appendListOfLists(rt->th_work[th].excl);
             }
-
             for (const thread_work_t& th_work : rt->th_work)
             {
                 *excl_count += th_work.excl_count;
index b411d6a602b2cba8438b8f561059e2b3fa4e9af2..5f6801a4dc67b7efb80d080c4df3a7f89b8add57 100644 (file)
@@ -180,7 +180,6 @@ static void nnb2excl(t_nextnb* nnb, gmx::ListOfLists<int>* excls)
 
     excls->clear();
 
-    std::vector<int> exclusionsForAtom;
     for (i = 0; (i < nnb->nr); i++)
     {
         /* calculate the total number of exclusions for atom i */
@@ -232,12 +231,12 @@ static void nnb2excl(t_nextnb* nnb, gmx::ListOfLists<int>* excls)
         prints("after rm-double", j_index, s);
 
         /* put the sorted exclusions in the target list */
-        exclusionsForAtom.clear();
+        excls->pushBackListOfSize(nr_of_sortables);
+        gmx::ArrayRef<int> exclusionsForAtom = excls->back();
         for (nrs = 0; (nrs < nr_of_sortables); nrs++)
         {
-            exclusionsForAtom.push_back(s[nrs].aj);
+            exclusionsForAtom[nrs] = s[nrs].aj;
         }
-        excls->pushBack(exclusionsForAtom);
 
         /* cleanup temporary space */
         sfree(s);
index 8ed07ece85b992c02c70ef9e88cd298422978ecc..bf2768b4146da30b8670dae73867d2f78a50a991 100644 (file)
@@ -45,6 +45,7 @@
 
 #include "bench_system.h"
 
+#include <numeric>
 #include <vector>
 
 #include "gromacs/math/vec.h"
@@ -168,7 +169,6 @@ BenchmarkSystem::BenchmarkSystem(const int multiplicationFactor)
     atomInfoAllVdw.resize(numAtoms);
     atomInfoOxygenVdw.resize(numAtoms);
 
-    std::vector<int> exclusionsForAtom;
     for (int a = 0; a < numAtoms; a++)
     {
         if (a % numAtomsInMolecule == 0)
@@ -189,13 +189,10 @@ BenchmarkSystem::BenchmarkSystem(const int multiplicationFactor)
         SET_CGINFO_HAS_Q(atomInfoAllVdw[a]);
         SET_CGINFO_HAS_Q(atomInfoOxygenVdw[a]);
 
-        exclusionsForAtom.clear();
-        const int firstAtomInMolecule = a - (a % numAtomsInMolecule);
-        for (int aj = 0; aj < numAtomsInMolecule; aj++)
-        {
-            exclusionsForAtom.push_back(firstAtomInMolecule + aj);
-        }
-        excls.pushBack(exclusionsForAtom);
+        excls.pushBackListOfSize(numAtomsInMolecule);
+        gmx::ArrayRef<int> exclusionsForAtom   = excls.back();
+        const int          firstAtomInMolecule = a - (a % numAtomsInMolecule);
+        std::iota(exclusionsForAtom.begin(), exclusionsForAtom.end(), firstAtomInMolecule);
     }
 
     forceRec.ntype = numAtomTypes;
index ec81ab55889f14a2fc8a1cbb706aa87d32ce577a..e32d8495099ab248196ae3c4336f016710f0e00b 100644 (file)
@@ -370,15 +370,14 @@ void ExclusionsHelper::generateExclusions()
     // particles would be higher, or where the exclusions would not be random,
     // to make a higher percentage of the exclusions to actually be within the
     // cutoff.
-    std::vector<int> exclusionsForAtom;
     for (int i = 0; i < testPosCount_; ++i)
     {
-        exclusionsForAtom.clear();
+        excls_.pushBackListOfSize(20);
+        gmx::ArrayRef<int> exclusionsForAtom = excls_.back();
         for (int j = 0; j < 20; ++j)
         {
-            exclusionsForAtom.push_back(i + j * 3);
+            exclusionsForAtom[j] = i + j * 3;
         }
-        excls_.pushBack(exclusionsForAtom);
     }
 }
 
index 53a2a05fbcb04605c034338b9e1eabcb09f3db09..0a45f88c03f3cbf7242b823a13a60a57e4523714 100644 (file)
@@ -43,6 +43,7 @@
 
 #include "gromacs/topology/exclusionblocks.h"
 
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 #include "gromacs/topology/block.h"
@@ -149,11 +150,8 @@ public:
         for (index i = 0; i < ssize(b_); i++)
         {
             gmx::ArrayRef<const int> jList = list_[i];
-            EXPECT_EQ(b_[i].nra(), jList.ssize()) << "Block size mismatch at " << i << ".";
-            for (int j = 0; j < b_[i].nra(); j++)
-            {
-                EXPECT_EQ(b_[i].atomNumber[j], jList[j]) << "Block mismatch at " << i << " , " << j << ".";
-            }
+            ASSERT_EQ(b_[i].nra(), jList.ssize()) << "Block size mismatch at " << i << ".";
+            EXPECT_THAT(b_[i].atomNumber, ::testing::Pointwise(::testing::Eq(), jList));
         }
     }
 
index c25a10c05da057c7f39ad074f9f26963f83fe283..96927dc80c882d3be40a355236a450e25f21d5b8 100644 (file)
@@ -67,11 +67,18 @@ namespace gmx
  * lists concatenated. List i is stored in entries listRanges_[i] to
  * listRanges_[i+1] in elements_.
  *
+ * \note This class is currently limited to arithmetic types, mainly because
+ * this should only be used for performance critical applications.
+ * When performance is not critical, a std::vector of std::vector can be used.
+ *
  * \tparam T value type
  */
+
 template<typename T>
 class ListOfLists
 {
+    static_assert(std::is_arithmetic<T>::value, "This class is limited to arithmetic types");
+
 public:
     //! Constructs an empty list of lists
     ListOfLists() = default;
@@ -122,6 +129,16 @@ public:
         listRanges_.push_back(int(elements_.size()));
     }
 
+    //! Appends a new list with \p numElements elements
+    void pushBackListOfSize(int numElements)
+    {
+        // With arithmetic types enforced, this assertion is always true
+        static_assert(std::is_default_constructible<T>::value,
+                      "pushBackListOfSize should only be called with default constructable types");
+        elements_.resize(elements_.size() + numElements);
+        listRanges_.push_back(int(elements_.size()));
+    }
+
     //! Returns an ArrayRef to the elements of the list with the given index
     ArrayRef<const T> operator[](std::size_t listIndex) const
     {
@@ -136,6 +153,29 @@ public:
                                  elements_.data() + listRanges_.at(listIndex + 1));
     }
 
+    /*! \brief Returns a reference to the first list
+     *
+     * \returns a reference to the first list
+     */
+    ArrayRef<T> front()
+    {
+        GMX_ASSERT(size() > 0, "Must contain a list if front() is called");
+        auto beginPtr = elements_.data();
+        auto endPtr   = beginPtr + listRanges_[1];
+        return { beginPtr, endPtr };
+    }
+    /*! \brief Returns a reference to the final list
+     *
+     * \returns a reference to the final list
+     */
+    ArrayRef<T> back()
+    {
+        GMX_ASSERT(size() > 0, "Must contain a list if bank() is called");
+        auto endIndex   = *(listRanges_.end() - 1);
+        auto beginIndex = *(listRanges_.end() - 2);
+        return { elements_.data() + beginIndex, elements_.data() + endIndex };
+    }
+
     //! Clears the list
     void clear()
     {
@@ -143,37 +183,24 @@ public:
         elements_.clear();
     }
 
-    //! Appends a ListOfLists at the end
-    void appendListOfLists(const ListOfLists& listOfLists)
+    //! Appends a ListOfLists at the end and increments the appended elements by \p offset
+    void appendListOfLists(const ListOfLists& listOfLists, const T offset = 0)
     {
-        const std::size_t oldNumLists = size();
         listRanges_.insert(listRanges_.end(), listOfLists.listRanges_.begin() + 1,
                            listOfLists.listRanges_.end());
         const int oldNumElements = elements_.size();
-        for (std::size_t i = oldNumLists + 1; i < listRanges_.size(); i++)
+        for (std::size_t i = listRanges_.size() - listOfLists.size(); i < listRanges_.size(); i++)
         {
             listRanges_[i] += oldNumElements;
         }
         elements_.insert(elements_.end(), listOfLists.elements_.begin(), listOfLists.elements_.end());
-    }
 
-    /*! \brief Appends a ListOfLists at the end and increments the appended elements by \p offset
-     *
-     * \tparam U  Type which should be the same as \p T
-     *
-     * Note that we can not rely on SFINAE for this void function without additional templating.
-     * So to enable compilation of ListOfLists for all types, we use a second template parameter
-     * which can be automatically deduced from \p listOfLists.
-     */
-    template<typename U>
-    std::enable_if_t<std::is_same<U, T>::value && std::is_arithmetic<T>::value, void>
-    appendListOfLists(const ListOfLists<U>& listOfLists, const T offset)
-    {
-        const std::size_t oldNumElements = elements_.size();
-        appendListOfLists(listOfLists);
-        for (std::size_t i = oldNumElements; i < elements_.size(); i++)
+        if (offset != 0)
         {
-            elements_[i] += offset;
+            for (std::size_t i = elements_.size() - listOfLists.elements_.size(); i < elements_.size(); i++)
+            {
+                elements_[i] += offset;
+            }
         }
     }
 
index 40b96d14c58b248955e5f3a18249a4a9bd30b31f..e36fb75df9fe49560a21b888026eca6a272afa13 100644 (file)
 
 #include "gromacs/utility/listoflists.h"
 
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 #include "testutils/testasserts.h"
-#include "testutils/testmatchers.h"
 
 namespace gmx
 {
@@ -54,13 +54,13 @@ namespace gmx
 namespace
 {
 
+using ::testing::Eq;
+using ::testing::Pointwise;
+
 //! Compares all element between two lists of lists
 template<typename T>
 void compareLists(const ListOfLists<T>& list, const std::vector<std::vector<T>>& v)
 {
-    using ::testing::Eq;
-    using ::testing::Pointwise;
-
     ASSERT_EQ(list.size(), v.size());
     for (std::size_t i = 0; i < list.size(); i++)
     {
@@ -92,21 +92,13 @@ void checkAppend(const std::vector<std::vector<T>> inputLists)
     compareLists(list, inputLists);
 }
 
-TEST(ListOfLists, AppendPodWorks)
+TEST(ListOfLists, AppendWorks)
 {
     const std::vector<std::vector<char>> v = { { 5, 3 }, { char(-1), 7, 4 } };
 
     checkAppend(v);
 }
 
-TEST(ListOfLists, AppendNonpodWorks)
-{
-    const std::vector<std::vector<std::string>> v = { { "Will", "this" },
-                                                      { "test", "work", "", "?" } };
-
-    checkAppend(v);
-}
-
 TEST(ListOfLists, EmptyListWorks)
 {
     ListOfLists<char> list;
@@ -119,6 +111,17 @@ TEST(ListOfLists, EmptyListWorks)
     EXPECT_EQ(a.empty(), true);
 }
 
+TEST(ListOfLists, AppendAccessWorks)
+{
+    const std::vector<std::vector<char>> v = { { 5, 3 }, { char(-1), 4 } };
+
+    ListOfLists<char> list;
+    list.pushBack(v[0]);
+    list.pushBackListOfSize(v[1].size());
+    std::copy(v[1].begin(), v[1].end(), list.back().begin());
+    compareLists(list, v);
+}
+
 TEST(ListOfLists, ClearWorks)
 {
     ListOfLists<char> list;
@@ -139,6 +142,42 @@ TEST(ListOfLists, OutOfRangeAccessThrows)
     EXPECT_THROW(list.at(1), std::out_of_range);
 }
 
+TEST(ListOfLists, FrontAndBackWork)
+{
+    ListOfLists<char> list1;
+    std::vector<char> v1{ { 3, 4 } };
+    list1.pushBack(v1);
+    EXPECT_THAT(list1.front(), Pointwise(Eq(), v1));
+    EXPECT_THAT(list1.back(), Pointwise(Eq(), v1));
+
+    std::vector<char> v2{ { 12, 63, 1 } };
+    list1.pushBack(v2);
+    EXPECT_THAT(list1.front(), Pointwise(Eq(), v1));
+    EXPECT_THAT(list1.back(), Pointwise(Eq(), v2));
+
+    list1.pushBack({});
+    EXPECT_THAT(list1.front(), Pointwise(Eq(), v1));
+    EXPECT_THAT(list1.back(), Pointwise(Eq(), std::vector<char>{}));
+
+    std::vector<char> v3{ { 99, 0, char(-1) } };
+    list1.pushBack(v3);
+    EXPECT_THAT(list1.front(), Pointwise(Eq(), v1));
+    EXPECT_THAT(list1.back(), Pointwise(Eq(), v3));
+
+    ListOfLists<char> list2;
+    list2.pushBack(v2);
+    EXPECT_THAT(list2.front(), Pointwise(Eq(), v2));
+    EXPECT_THAT(list2.back(), Pointwise(Eq(), v2));
+
+    list2.appendListOfLists(list1);
+    EXPECT_THAT(list2.front(), Pointwise(Eq(), v2));
+    EXPECT_THAT(list2.back(), Pointwise(Eq(), v3));
+    EXPECT_EQ(list2.back().size(), v3.size());
+
+    list2.pushBackListOfSize(1);
+    EXPECT_EQ(list2.back().size(), 1);
+}
+
 TEST(ListOfLists, ExtractsAndRestores)
 {
     const std::vector<std::vector<char>> v({ { 5, 3 }, {}, { char(-1), 4 } });
@@ -181,21 +220,6 @@ TEST(ListOfLists, AppendsListOfListsWithOffset)
     compareLists(list1, v);
 }
 
-TEST(ListOfLists, AppendsListOfListsNonpod)
-{
-    std::vector<std::vector<std::string>> v({ { "a", "bc" }, { "d" }, {}, { "efg", "h" } });
-
-    ListOfLists<std::string> list1;
-    ListOfLists<std::string> list2;
-
-    list1.pushBack(v[0]);
-    list1.pushBack(v[1]);
-    list2.pushBack(v[2]);
-    list2.pushBack(v[3]);
-    list1.appendListOfLists(list2);
-    compareLists(list1, v);
-}
-
 } // namespace
 
 } // namespace gmx