From 4ca12173920bbbdeea08c90bf6670932dc96a1d8 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Fri, 31 Mar 2023 11:58:14 +0100 Subject: [PATCH 01/25] added size_all() to IndexRange --- src/include/stir/IndexRange.h | 5 +++++ src/include/stir/IndexRange.inl | 15 +++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/include/stir/IndexRange.h b/src/include/stir/IndexRange.h index e16bbc9575..882cfb33cc 100644 --- a/src/include/stir/IndexRange.h +++ b/src/include/stir/IndexRange.h @@ -90,6 +90,9 @@ class IndexRange : public VectorWithOffset> //! Construct a regular range given by sizes (minimum indices will be 0) inline IndexRange(const BasicCoordinate& sizes); + //! return the total number of elements in this range + inline size_t size_all() const; + // these are derived from VectorWithOffset // TODO these should be overloaded, to set regular_range as well. /* @@ -139,6 +142,8 @@ class IndexRange<1> inline int get_min_index() const; inline int get_max_index() const; inline int get_length() const; + //! return the total number of elements in this range + inline size_t size_all() const; inline bool operator==(const IndexRange<1>& range2) const; diff --git a/src/include/stir/IndexRange.inl b/src/include/stir/IndexRange.inl index 2e981cbd48..aeb62d3563 100644 --- a/src/include/stir/IndexRange.inl +++ b/src/include/stir/IndexRange.inl @@ -66,6 +66,17 @@ IndexRange::IndexRange(const BasicCoordinatefill(lower_dims); } +template +std::size_t +IndexRange::size_all() const +{ + this->check_state(); + size_t acc=0; + for(int i=this->get_min_index(); i<=this->get_max_index(); i++) + acc += this->num[i].size_all(); + return acc; +} + template bool IndexRange::operator==(const IndexRange& range2) const @@ -150,6 +161,10 @@ IndexRange<1>::get_length() const return max - min + 1; } +std::size_t +IndexRange<1>::size_all() const +{ return std::size_t(this->get_length()); } + bool IndexRange<1>::operator==(const IndexRange<1>& range2) const { From 1873c9df5f45b670ce42df1be20a4d4b8be85c22 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Fri, 31 Mar 2023 12:10:03 +0100 Subject: [PATCH 02/25] allow Vector/Array construction from existing memory needs more tests --- src/include/stir/Array.h | 81 ++++++++++++- src/include/stir/Array.inl | 116 +++++++++++++++++++ src/include/stir/NumericVectorWithOffset.h | 3 + src/include/stir/NumericVectorWithOffset.inl | 6 + src/include/stir/VectorWithOffset.h | 18 ++- src/include/stir/VectorWithOffset.inl | 32 ++++- src/test/test_Array.cxx | 95 ++++++++++++++- 7 files changed, 335 insertions(+), 16 deletions(-) diff --git a/src/include/stir/Array.h b/src/include/stir/Array.h index 837ae7be47..04c3a77abc 100644 --- a/src/include/stir/Array.h +++ b/src/include/stir/Array.h @@ -3,6 +3,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2011-10-14, Hammersmith Imanet Ltd Copyright (C) 2011-07-01 - 2012, Kris Thielemans + Copyright (C) 2023, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license @@ -34,6 +35,7 @@ #include "stir/ByteOrder.h" #include "stir/IndexRange.h" #include "stir/deprecated.h" +#include "stir/shared_ptr.h" START_NAMESPACE_STIR class NumericType; @@ -148,6 +150,16 @@ class Array : public NumericVectorWithOffset, e //! virtual destructor, frees up any allocated memory inline ~Array() override; + //! change the array to a new range of indices, pointing to \c data_ptr + /*! + \arg data_ptr should point to a contiguous block of correct size + + The C-array \data_ptr will be accessed with the last dimension running fastest + ("row-major" order). + */ + inline virtual void + init(const IndexRange& range, elemT * const data_ptr, bool copy_data); + /*! @name functions returning full_iterators*/ //@{ //! start value for iterating through all elements in the array, see full_iterator @@ -169,14 +181,18 @@ class Array : public NumericVectorWithOffset, e //! return the total number of elements in this array inline size_t size_all() const; - /* Implementation note: grow() and resize() are inline such that they are - defined for any type you happen to use for elemT. Otherwise, we would - need instantiation in Array.cxx. - */ //! change the array to a new range of indices, new elements are set to 0 + /*! Current behaviour is that when resizing to a smaller array, the same memory + will be used. However, when growing any of the dimensions, a new Array + will be allocated and the data copied. + + If the array points to an existing block of data, resizing is therefore problematic. + When growing the array, the resized array will no longer point to the original block + of data. + */ inline virtual void resize(const IndexRange& range); - //! grow the array to a new range of indices, new elements are set to 0 + //! alias for resize() virtual inline void grow(const IndexRange& range); //! return sum of all elements @@ -250,6 +266,29 @@ class Array : public NumericVectorWithOffset, e //! set values of the array to self*a+y*b where a and b are scalar or arrays template inline void sapyb(const T& a, const Array& y, const T& b); + + //! \name access to the data via a pointer + //@{ + //! return if the array is contiguous in memory + bool is_contiguous() const; + + //! member function for access to the data via a elemT* + inline elemT* get_full_data_ptr(); + + //! member function for access to the data via a const elemT* + inline const elemT* get_const_full_data_ptr() const; + + //! signal end of access to elemT* + inline void release_full_data_ptr(); + + //! signal end of access to const elemT* + inline void release_const_full_data_ptr() const; + //@} + + private: + //! boolean to test if get_full_data_ptr is called + // This variable is declared mutable such that get_const_full_data_ptr() can change it. + mutable bool _full_pointer_access; }; /************************************************** @@ -307,12 +346,27 @@ class Array<1, elemT> : public NumericVectorWithOffset //! constructor given first and last indices, initialising elements to 0 inline Array(const int min_index, const int max_index); + //! constructor given an IndexRange<1>, pointing to \c data_ptr + inline Array(const IndexRange<1>& range, elemT * const data_ptr ); + + //! constructor given first and last indices, pointing to \c data_ptr + /*! + \arg data_ptr should start to a contiguous block of correct size + */ + inline Array(const int min_index, const int max_index, elemT * const data_ptr); + //! constructor from basetype inline Array(const NumericVectorWithOffset& il); //! virtual destructor inline ~Array() override; + //! change vector with new index range and point to \c data_ptr + /*! + \arg data_ptr should start to a contiguous block of correct size + */ + inline void init(const IndexRange<1>& range, elemT * const data_ptr, bool copy_data); + /*! @name functions returning full_iterators*/ //@{ //! start value for iterating through all elements in the array, see full_iterator @@ -347,6 +401,23 @@ class Array<1, elemT> : public NumericVectorWithOffset // Array::resize initialises new elements to 0 inline void resize(const int min_index, const int max_index) override; + //! \name access to the data via a pointer + //@{ + //! return if the array is contiguous in memory (always \c true) + bool is_contiguous() const { return true; } + //! member function for access to the data via a elemT* + inline elemT* get_full_data_ptr() { return this->get_data_ptr(); } + + //! member function for access to the data via a const elemT* + inline const elemT* get_const_full_data_ptr() const { return this->get_const_data_ptr(); } + + //! signal end of access to elemT* + inline void release_full_data_ptr() { this->release_data_ptr(); } + + //! signal end of access to const elemT* + inline void release_const_full_data_ptr() const { this->release_const_data_ptr(); } + //@} + //! return sum of all elements inline elemT sum() const; diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index 98fce7eb12..dc20523b04 100644 --- a/src/include/stir/Array.inl +++ b/src/include/stir/Array.inl @@ -4,6 +4,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2011-01-11, Hammersmith Imanet Ltd Copyright (C) 2011-07-01 - 2012, Kris Thielemans + Copyright (C) 2023, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license @@ -30,6 +31,21 @@ START_NAMESPACE_STIR /********************************************** inlines for Array **********************************************/ +template +bool +Array::is_contiguous() const +{ + auto mem = &(*this->begin_all()); + for (auto i=this->get_min_index(); iget_max_index(); ++i) + { + if (!(*this)[i].is_contiguous()) + return false; + mem += (*this)[i].size_all(); + if (mem != &(*(*this)[i+1].begin_all())) + return false; + } + return true; +} template void @@ -44,6 +60,23 @@ Array::resize(const IndexRange& range) template void +Array::init(const IndexRange& range, elemT * const data_ptr, bool copy_data) +{ + base_type::resize(range.get_min_index(), range.get_max_index()); + auto iter = this->begin(); + auto range_iter = range.begin(); + auto ptr = data_ptr; + for (; + iter != this->end(); + ++iter, ++range_iter) + { + (*iter).init(*range_iter, ptr, copy_data); + ptr += range_iter->size_all(); + } +} + +template +void Array::grow(const IndexRange& range) { resize(range); @@ -151,6 +184,7 @@ Array::get_index_range() const } return IndexRange(range); } + template size_t Array::size_all() const @@ -162,6 +196,81 @@ Array::size_all() const return acc; } +/*! + If is_contiguous() is \c false, calls error(). Otherwise, return a + \c elemT* to the first element of the array. + + Use only in emergency cases... + + To prevent invalidating the safety checks (and making + reimplementation more difficult), NO manipulation with + the array is allowed between the pairs + get_full_data_ptr() and release_full_data_ptr() + and + get_const_full_data_ptr() and release_const_full_data_ptr(). + (This is checked with assert() in DEBUG mode.) +*/ +template +elemT* +Array::get_full_data_ptr() +{ + this->_full_pointer_access = true; + if (!this->is_contiguous()) + error("Array::get_full_data_ptr() called for non-contiguous array."); + return &(*this->begin_all()); +}; + +/*! + If is_contiguous() is \c false, calls error(). Otherwise, return a + \c const \c elemT* to the first element of the array. + + Use get_const_full_data_ptr() when you are not going to modify + the data. + + \see get_full_data_ptr() +*/ +template +const elemT * +Array::get_const_full_data_ptr() const +{ + this->_full_pointer_access = true; + if (!this->is_contiguous()) + error("Array::get_const_full_data_ptr() called for non-contiguous array."); + return &(*this->begin_all_const()); +}; + +/*! + This has to be used when access to the elemT* returned by get_full_data_ptr() is + finished. It updates + the Array with any changes you made, and allows access to + the other member functions again. + + \see get_full_data_ptr() +*/ +template +void +Array::release_full_data_ptr() +{ + assert(this->_full_pointer_access); + + this->_full_pointer_access = false; +} + +/*! + This has to be used when access to the const elemT* returned by get_const_full_data_ptr() is + finished. It allows access to the other member functions again. + + \see get_const_full_data_ptr() +*/ + +template +void +Array::release_const_full_data_ptr() const +{ + assert(this->_full_pointer_access); + this->_full_pointer_access = false; +} + template elemT Array::sum() const @@ -384,6 +493,13 @@ Array::sapyb(const T& a, const Array& y, const T& b) /********************************************** inlines for Array<1, elemT> **********************************************/ +template +void +Array<1, elemT>::init(const IndexRange<1>& range, elemT * const data_ptr, bool copy_data) +{ + base_type:: + init(range.get_min_index(), range.get_max_index(), data_ptr, copy_data); +} template void diff --git a/src/include/stir/NumericVectorWithOffset.h b/src/include/stir/NumericVectorWithOffset.h index 0d13836681..35fdd97a58 100644 --- a/src/include/stir/NumericVectorWithOffset.h +++ b/src/include/stir/NumericVectorWithOffset.h @@ -59,6 +59,9 @@ class NumericVectorWithOffset : public VectorWithOffset //! Construct a NumericVectorWithOffset of elements with offset \c min_index inline NumericVectorWithOffset(const int min_index, const int max_index); + //! Construct a NumericVectorWithOffset of elements with offset \c min_index pointing to \c data_ptr + inline NumericVectorWithOffset(const int min_index, const int max_index, elemT * const data_ptr); + //! Constructor from an object of this class' base_type inline NumericVectorWithOffset(const VectorWithOffset& t); diff --git a/src/include/stir/NumericVectorWithOffset.inl b/src/include/stir/NumericVectorWithOffset.inl index 0371ac205f..f452169425 100644 --- a/src/include/stir/NumericVectorWithOffset.inl +++ b/src/include/stir/NumericVectorWithOffset.inl @@ -42,6 +42,12 @@ inline NumericVectorWithOffset::NumericVectorWithOffset(const int min : base_type(min_index, max_index) {} +template +inline +NumericVectorWithOffset::NumericVectorWithOffset(const int min_index, const int max_index, NUMBER * const data_ptr) + : base_type(min_index, max_index, data_ptr) +{} + template NumericVectorWithOffset::NumericVectorWithOffset(const VectorWithOffset& t) : base_type(t) diff --git a/src/include/stir/VectorWithOffset.h b/src/include/stir/VectorWithOffset.h index 81b8c85d83..3f1ec8c375 100644 --- a/src/include/stir/VectorWithOffset.h +++ b/src/include/stir/VectorWithOffset.h @@ -4,10 +4,10 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2007-10-08, Hammersmith Imanet Ltd Copyright (C) 2012-06-01 - 2012, Kris Thielemans + Copyright (C) 2023, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license - Copyright (C) 2000- 2012, Hammersmith Imanet Ltd See STIR/LICENSE.txt for details */ #ifndef __stir_VectorWithOffset_H__ @@ -138,11 +138,11 @@ class VectorWithOffset //! Construct a VectorWithOffset with offset \c min_index (initialised with \c T()) inline VectorWithOffset(const int min_index, const int max_index); - //! Construct a VectorWithOffset of given length using existing data (no initialisation) - inline explicit VectorWithOffset(const int hsz, T* const data_ptr, T* const end_of_data_ptr); - - //! Construct a VectorWithOffset with offset \c min_index using existing data (no initialisation) - inline VectorWithOffset(const int min_index, const int max_index, T* const data_ptr, T* const end_of_data_ptr); + //! Construct a VectorWithOffset of given length pointing to existing data + inline explicit VectorWithOffset(const int hsz, T * const data_ptr); + + //! Construct a VectorWithOffset with offset \c min_index point to existing data + inline VectorWithOffset(const int min_index, const int max_index, T * const data_ptr); //! copy constructor inline VectorWithOffset(const VectorWithOffset& il); @@ -150,6 +150,12 @@ class VectorWithOffset //! Destructor inline virtual ~VectorWithOffset(); + //! change vector with new index range and point to \c data_ptr + /*! + \arg data_ptr should start to a contiguous block of correct size + */ + inline void init(const int min_index, const int max_index, T * const data_ptr, bool copy_data); + //! Free all memory and make object as if default-constructed /*! This is not the same as resize(0), as the latter does not deallocate the memory (i.e. does not change the capacity()). diff --git a/src/include/stir/VectorWithOffset.inl b/src/include/stir/VectorWithOffset.inl index 71193eaa38..bce23e1117 100644 --- a/src/include/stir/VectorWithOffset.inl +++ b/src/include/stir/VectorWithOffset.inl @@ -4,6 +4,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2010-07-01, Hammersmith Imanet Ltd Copyright (C) 2012-06-01 - 2012, Kris Thielemans + Copyright (C) 2023, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license @@ -21,6 +22,7 @@ */ +#include "stir/IndexRange.h" #include #include #include "thresholding.h" @@ -39,6 +41,28 @@ VectorWithOffset::init() end_allocated_memory = 0; } +template +void +VectorWithOffset::init(const int min_index, const int max_index, T * const data_ptr, bool copy_data) +{ + this->pointer_access = false; + this->_owns_memory_for_data = copy_data; + if (copy_data) + { + this->resize(min_index, max_index); + std::copy(data_ptr, data_ptr + this->length, this->begin()); + } + else + { + this->length = static_cast(max_index - min_index) + 1; + this->start = min_index; + this->begin_allocated_memory = data_ptr; + this->end_allocated_memory = data_ptr + this->length; + this->num = this->begin_allocated_memory - this->start; + this->check_state(); + } +} + template bool VectorWithOffset::owns_memory_for_data() const @@ -266,27 +290,27 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index) } template -VectorWithOffset::VectorWithOffset(const int sz, T* const data_ptr, T* const end_of_data_ptr) +VectorWithOffset::VectorWithOffset(const int sz, T * const data_ptr) : length(static_cast(sz)), start(0), pointer_access(false), _owns_memory_for_data(false) { this->begin_allocated_memory = data_ptr; - this->end_allocated_memory = end_of_data_ptr; + this->end_allocated_memory = data_ptr + sz; this->num = this->begin_allocated_memory - this->start; this->check_state(); } template -VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, T* const data_ptr, T* const end_of_data_ptr) +VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, T * const data_ptr) : length(static_cast(max_index - min_index) + 1), start(min_index), pointer_access(false), _owns_memory_for_data(false) { this->begin_allocated_memory = data_ptr; - this->end_allocated_memory = end_of_data_ptr; + this->end_allocated_memory = data_ptr + (max_index - min_index + 1); this->num = this->begin_allocated_memory - this->start; this->check_state(); } diff --git a/src/test/test_Array.cxx b/src/test/test_Array.cxx index 0baebf8273..7ce8add8de 100644 --- a/src/test/test_Array.cxx +++ b/src/test/test_Array.cxx @@ -2,7 +2,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000-2011, Hammersmith Imanet Ltd Copyright (C) 2013 Kris Thielemans - Copyright (C) 2013, 2020 University College London + Copyright (C) 2013, 2020, 2023 University College London This file is part of STIR. @@ -344,6 +344,56 @@ ArrayTests::run_tests() Array<1, float> test3 = test2 + test; check_if_zero(test3[-2], "test growing during operator+"); } + + // using preallocated memory + { + std::vector mem(test.get_index_range().size_all()); + std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); + Array<1,float> preallocated; + preallocated.init(test.get_index_range(), &mem[0], false); + check_if_equal(test, preallocated, "test preallocated: equality"); + std::copy(test.begin_all_const(), test.end_all_const(), preallocated.begin_all()); + check_if_equal(test, preallocated, "test preallocated: copy with full_iterator"); + check(test.is_contiguous(), "test Array1D is contiguous"); + check(preallocated.is_contiguous(), "test Array1D is contiguous (preallocated)"); + check(preallocated.get_full_data_ptr() == &mem[0], "test Array1D preallocated pointer access"); + preallocated.release_full_data_ptr(); + check(preallocated.get_const_full_data_ptr() == &mem[0], "test Array1D preallocated const pointer access"); + preallocated.release_const_full_data_ptr(); + // test memory is shared between the Array and mem + mem[0]=*test.begin() + 345; + check_if_equal(*preallocated.begin(), mem[0], "test preallocated: direct buffer mod"); + *(preallocated.begin()+1) += 4; + check_if_equal(*(preallocated.begin()+1), mem[1], "test preallocated: indirect buffer mod"); + // test resize + { + const auto min = preallocated.get_min_index(); + const auto max = preallocated.get_max_index(); + // resizing to smaller range will keep pointing to the same memory + preallocated.resize(min+1, max-1); + std::fill(mem.begin(), mem.end(), 12345.F); + check_if_equal(preallocated[min+1], 12345.F, "test preallocated: resize smaller uses same memory"); + // resizing to non-overlapping range will reallocate + preallocated.resize(min-1, max-1); + std::fill(mem.begin(), mem.end(), 123456.F); + check_if_equal(preallocated[min+1], 12345.F, "test preallocated: grow uses different memory"); + } + } + // copying from existing memory + { + std::vector mem(test.get_index_range().size_all()); + std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); + Array<1,float> test_from_mem; + test_from_mem.init(test.get_index_range(), &mem[0], true); + check_if_equal(test, test_from_mem, "test construct from mem: equality"); + std::copy(test.begin_all_const(), test.end_all_const(), test_from_mem.begin_all()); + check_if_equal(test, test_from_mem, "test construct from mem: copy with full_iterator"); + // test memory is not shared between the Array and mem + mem[0]=*test.begin() + 345; + check_if_equal(*test_from_mem.begin(), *test.begin(), "test construct from mem: direct buffer mod"); + *(test_from_mem.begin()+1) += 4; + check_if_equal(*(test_from_mem.begin()+1), mem[1]+4, "test construct from mem: indirect buffer mod"); + } } #if 1 { @@ -456,6 +506,49 @@ ArrayTests::run_tests() Array<2, float> t22 = t2; check_if_equal(t22, t2, "test Array2D copy constructor"); } + + // using preallocated memory + { + std::vector mem(t2.get_index_range().size_all()); + std::copy(t2.begin_all_const(), t2.end_all_const(), mem.begin()); + { + // test iterator access is row-major + auto first_min_idx = t2.get_min_index(); + check_if_equal(t2[3][2], + mem[(3 - first_min_idx) * t2[first_min_idx].size_all() + + 2 - t2[first_min_idx].get_min_index()], + "check row-major order in 2D"); + } + Array<2,float> preallocated; + preallocated.init(t2.get_index_range(), &mem[0], false); + check_if_equal(t2, preallocated, "test preallocated: equality"); + std::copy(t2.begin_all_const(), t2.end_all_const(), preallocated.begin_all()); + check_if_equal(t2, preallocated, "test preallocated: copy with full_iterator"); + + check(preallocated.is_contiguous(), "test Array2D is contiguous (preallocated)"); + check(preallocated.get_full_data_ptr() == &mem[0], "test Array2D preallocated pointer access"); + preallocated.release_full_data_ptr(); + check(preallocated.get_const_full_data_ptr() == &mem[0], "test Array2D preallocated const pointer access"); + preallocated.release_const_full_data_ptr(); + // test memory is shared between the Array and mem + mem[0]=*t2.begin_all() + 345; + check_if_equal(*preallocated.begin_all(), mem[0], "test preallocated: direct buffer mod"); + *(preallocated.begin_all()) += 4; + check_if_equal(*(preallocated.begin_all()), mem[0], "test preallocated: indirect buffer mod"); + // test resize + { + BasicCoordinate<2,int> min, max; + preallocated.get_regular_range(min, max); + // resizing to smaller range will keep pointing to the same memory + preallocated.resize(IndexRange<2>(min+1, max-1)); + std::fill(mem.begin(), mem.end(), 12345.F); + check_if_equal(preallocated[min+1], 12345.F, "test preallocated: resize smaller uses same memory"); + check(!preallocated.is_contiguous(), "test preallocated: no longer contiguous after resize"); + preallocated.resize(IndexRange<2>(min-1, max-1)); + std::fill(mem.begin(), mem.end(), 123456.F); + check_if_equal(preallocated[min+1], 12345.F, "test preallocated: grow uses different memory"); + } + } } // size_all with irregular range { From 3e9527bcdb48c9f2aab50097bee14a7b4e960acd Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Tue, 18 Apr 2023 20:24:52 +0100 Subject: [PATCH 03/25] add some timings to test_Array (to be disabled later) --- src/test/test_Array.cxx | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/test/test_Array.cxx b/src/test/test_Array.cxx index 7ce8add8de..bfde52932c 100644 --- a/src/test/test_Array.cxx +++ b/src/test/test_Array.cxx @@ -48,6 +48,8 @@ // for open_read/write_binary #include "stir/utilities.h" +#include "stir/HighResWallClockTimer.h" + #include #include #include @@ -982,6 +984,42 @@ ArrayTests::run_tests() check_if_equal(arr1, arr3, "make_array inline vs function with assignment"); check_if_equal(arr1, arr4, "make_array inline constructor from function"); } + std::cerr << "timings\n"; + { + HighResWallClockTimer t; + IndexRange<4> range(Coordinate4D(20,100,400,600)); + t.start(); + double create_duration; + { + Array<4,int> a1(range); + t.stop(); + std::cerr << "creation of non-contiguous 4D Array " << t.value()*1000 << "ms\n"; + create_duration = t.value(); + t.start(); + } + t.stop(); + std::cerr << "deletion " << (t.value() - create_duration)*1000 << " ms\n"; + t.reset(); + t.start(); + { + const auto s = range.size_all(); + t.stop(); + std::cerr << "range size_all " << t.value()*1000 << "ms\n"; + t.start(); + std::vector v(s, 0); + t.stop(); + std::cerr << "vector creation " << t.value()*1000 << "ms\n"; + t.start(); + Array<4,int> a1; + a1.init(range, v.data(), false); + t.stop(); + create_duration = t.value(); + std::cerr << "contiguous array creation (total) " << t.value()*1000 << "ms\n"; + t.start(); + } + t.stop(); + std::cerr << "deletion " << (t.value() - create_duration)*1000 << " ms\n"; + } } END_NAMESPACE_STIR From 77cbf5a2fe67a5b8b5a6db6be17733fccad5edf4 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Sun, 20 Aug 2023 19:50:33 +0100 Subject: [PATCH 04/25] speed-up IndexRange::size_all for regular ranges --- src/include/stir/IndexRange.inl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/include/stir/IndexRange.inl b/src/include/stir/IndexRange.inl index aeb62d3563..604f62eff0 100644 --- a/src/include/stir/IndexRange.inl +++ b/src/include/stir/IndexRange.inl @@ -71,6 +71,9 @@ std::size_t IndexRange::size_all() const { this->check_state(); + if (this->is_regular_range == regular_true && this->get_length()>0) + return this->get_length() * this->begin()->size_all(); + //else size_t acc=0; for(int i=this->get_min_index(); i<=this->get_max_index(); i++) acc += this->num[i].size_all(); From ae72749d8def2a248af7e6b22846ae18d0e42373 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Mon, 21 Aug 2023 10:05:21 +0100 Subject: [PATCH 05/25] Array: first version that defaults to continuous allocation Currently only applies to the constructor from an IndexRange. Tests fail due to missing assignment operator. Best to use the copy-and-swap idiom. --- src/include/stir/Array.h | 14 ++++++++++---- src/include/stir/Array.inl | 37 ++++++++++++++++++++++++++++--------- 2 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/include/stir/Array.h b/src/include/stir/Array.h index 04c3a77abc..b259409da2 100644 --- a/src/include/stir/Array.h +++ b/src/include/stir/Array.h @@ -137,16 +137,19 @@ class Array : public NumericVectorWithOffset, e inline explicit Array(const IndexRange&); #ifndef SWIG - //! Construct an Array from an object of its base_type - inline Array(const base_type& t); -#else // swig 2.0.4 gets confused by base_type (due to numeric template arguments) // therefore, we declare this constructor using the "self" type, // i.e. it's just a copy-constructor. // This is less powerful as in C++, but swig-generated interfaces don't need to know about the base_type anyway - inline Array(const self& t); + //! Construct an Array from an object of its base_type + inline Array(const base_type& t); #endif + //! Copy constructor + // implementation needed as the above doesn't disable the auto-generated copy-constructor + inline Array(const self& t); + + //! virtual destructor, frees up any allocated memory inline ~Array() override; @@ -289,6 +292,9 @@ class Array : public NumericVectorWithOffset, e //! boolean to test if get_full_data_ptr is called // This variable is declared mutable such that get_const_full_data_ptr() can change it. mutable bool _full_pointer_access; + + //! A pointer to the allocated chunk if the array is constructed that way, zero otherwise + elemT* _allocated_full_data_ptr; }; /************************************************** diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index dc20523b04..9de604c378 100644 --- a/src/include/stir/Array.inl +++ b/src/include/stir/Array.inl @@ -25,6 +25,8 @@ #include #include "stir/assign.h" #include "stir/error.h" +//#include "stir/info.h" +//#include START_NAMESPACE_STIR @@ -84,29 +86,46 @@ Array::grow(const IndexRange& range) template Array::Array() - : base_type() + : base_type(), _allocated_full_data_ptr(0) {} template Array::Array(const IndexRange& range) - : base_type() + : base_type(), _allocated_full_data_ptr(0) { - grow(range); + //grow(range); + this->_allocated_full_data_ptr = new elemT[range.size_all()]; + // info("Array constructor range " + std::to_string(reinterpret_cast(this->_allocated_full_data_ptr)) + " of size " + std::to_string(range.size_all())); + std::fill(this->_allocated_full_data_ptr, this->_allocated_full_data_ptr + range.size_all(), elemT(0)); + this->init(range, this->_allocated_full_data_ptr, false); } template -#ifdef SWIG -// swig-specific work-around (see Array.h) Array::Array(const self& t) -#else +: base_type(t), _allocated_full_data_ptr(0) +{ + // info("constructor copy of size " + std::to_string(this->size_all())); +} + +#ifndef SWIG +// swig cannot parse this ATM, but we don't need it anyway in the wrappers +template Array::Array(const base_type& t) +: base_type(t), _allocated_full_data_ptr(0) +{ + // info("constructor basetype of size " + std::to_string(this->size_all())); +} #endif -: base_type(t) -{} template Array::~Array() -{} +{ + if (this->_allocated_full_data_ptr) + { + // info("Array destructor full_data_ptr " + std::to_string(reinterpret_cast(this->_allocated_full_data_ptr)) + " of size " + std::to_string(this->size_all())); + delete [] this->_allocated_full_data_ptr; + } +} template typename Array::full_iterator From 616dbb523f8b7c10071209ebaaef2f68684f323e Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Wed, 23 Aug 2023 11:55:23 +0100 Subject: [PATCH 06/25] Array: add move constructors, swap and missing assignment operators Most of the implementation uses the copy-and-swap idiom. I needed copy-constructors as well, as the default generated ones no longer existed or were inappropriate. We should now be following "the rule of five". This now passes test_Array, including with valgrind. --- src/include/stir/Array.h | 40 +++++++++++++++++++- src/include/stir/Array.inl | 38 +++++++++++++++++++ src/include/stir/NumericVectorWithOffset.h | 21 +++++++++- src/include/stir/NumericVectorWithOffset.inl | 22 ++++++++++- src/include/stir/VectorWithOffset.h | 19 ++++++++++ src/include/stir/VectorWithOffset.inl | 19 ++++++++++ 6 files changed, 155 insertions(+), 4 deletions(-) diff --git a/src/include/stir/Array.h b/src/include/stir/Array.h index b259409da2..b5387b2933 100644 --- a/src/include/stir/Array.h +++ b/src/include/stir/Array.h @@ -149,10 +149,28 @@ class Array : public NumericVectorWithOffset, e // implementation needed as the above doesn't disable the auto-generated copy-constructor inline Array(const self& t); - //! virtual destructor, frees up any allocated memory inline ~Array() override; + //! Swap content/members of 2 objects + // implementation in .h because of templates/friends/whatever, see https://stackoverflow.com/a/61020224 + friend inline void swap(Array& first, Array& second) // nothrow + { + using std::swap; + // swap the members of two objects + swap(static_cast(first), static_cast(second)); + swap(first._allocated_full_data_ptr, second._allocated_full_data_ptr); + } + + //! move constructor + /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ + Array(Array&& other) noexcept; + + //! assignment operator + /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ + Array& operator=(Array other); + + //! change the array to a new range of indices, pointing to \c data_ptr /*! \arg data_ptr should point to a contiguous block of correct size @@ -364,9 +382,29 @@ class Array<1, elemT> : public NumericVectorWithOffset //! constructor from basetype inline Array(const NumericVectorWithOffset& il); + //! Copy constructor + // implementation needed as the above doesn't replace the normal copy-constructor + // and the auto-generated is disabled because of the move constructor + inline Array(const self& t); + + //! move constructor + /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ + Array(Array&& other) noexcept; + //! virtual destructor inline ~Array() override; + //! Swap content/members of 2 objects + // implementation in .h because of templates/friends/whatever, see https://stackoverflow.com/a/61020224 + friend inline void swap(Array& first, Array& second) // nothrow + { + swap(static_cast(first), static_cast(second)); + } + + //! assignment + inline Array& operator=(const Array& other); + + //! change vector with new index range and point to \c data_ptr /*! \arg data_ptr should start to a contiguous block of correct size diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index 9de604c378..57390663fc 100644 --- a/src/include/stir/Array.inl +++ b/src/include/stir/Array.inl @@ -127,6 +127,22 @@ Array::~Array() } } +template +Array::Array(Array&& other) noexcept + : Array() +{ + swap(*this, other); +} + +template +Array& +Array:: +operator=(Array other) +{ + swap(*this, other); + return *this; +} + template typename Array::full_iterator Array::end_all() @@ -589,10 +605,32 @@ Array<1, elemT>::Array(const base_type& il) : base_type(il) {} +template +Array<1, elemT>::Array(const Array<1, elemT>& other) + : base_type(other) +{} + template Array<1, elemT>::~Array() {} +template +Array<1, elemT>::Array(Array<1, elemT>&& other) noexcept + : Array() +{ + swap(*this, other); +} + +template +Array<1, elemT>& +Array<1, elemT>::operator=(const Array<1, elemT>& other) +{ + // use the base_type assignment, as this tries to avoid reallocating memory + base_type::operator=(other); + return *this; +} + + template typename Array<1, elemT>::full_iterator Array<1, elemT>::begin_all() diff --git a/src/include/stir/NumericVectorWithOffset.h b/src/include/stir/NumericVectorWithOffset.h index 35fdd97a58..05b91a063f 100644 --- a/src/include/stir/NumericVectorWithOffset.h +++ b/src/include/stir/NumericVectorWithOffset.h @@ -4,7 +4,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2005-06-03, Hammersmith Imanet Ltd Copyright (C) 2011-07-01 - 2012, Kris Thielemans - Copyright (C) 2020, UCL + Copyright (C) 2020, 2023 UCL This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license @@ -65,6 +65,25 @@ class NumericVectorWithOffset : public VectorWithOffset //! Constructor from an object of this class' base_type inline NumericVectorWithOffset(const VectorWithOffset& t); + //! Constructor from an object of this class' base_type + inline NumericVectorWithOffset(const NumericVectorWithOffset& t) + : NumericVectorWithOffset(static_cast(t)) + {} + + //! Swap content/members of 2 objects + // implementation in .h because of templates/friends/whatever, see https://stackoverflow.com/a/61020224 + friend inline void swap(NumericVectorWithOffset& first, NumericVectorWithOffset& second) // nothrow + { + swap(static_cast(first), static_cast(second)); + } + + //! move constructor + /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ + NumericVectorWithOffset(NumericVectorWithOffset&& other) noexcept; + + //! assignment + NumericVectorWithOffset& operator=(const NumericVectorWithOffset& other); + // arithmetic operations with a vector, combining element by element //! adding vectors, element by element diff --git a/src/include/stir/NumericVectorWithOffset.inl b/src/include/stir/NumericVectorWithOffset.inl index f452169425..0cb5be5024 100644 --- a/src/include/stir/NumericVectorWithOffset.inl +++ b/src/include/stir/NumericVectorWithOffset.inl @@ -4,7 +4,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2005-06-03, Hammersmith Imanet Ltd Copyright (C) 2011-07-01 - 2012, Kris Thielemans - Copyright (C) 2013, 2020 University College London + Copyright (C) 2013, 2020, 2023 University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license @@ -14,7 +14,7 @@ /*! \file \ingroup Array - \brief inline implementations for NumericVectorWithOffset + \brief inline implementations for stir::NumericVectorWithOffset \author Kris Thielemans \author PARAPET project @@ -53,6 +53,24 @@ NumericVectorWithOffset::NumericVectorWithOffset(const VectorWithOffs : base_type(t) {} +template +NumericVectorWithOffset:: +NumericVectorWithOffset(NumericVectorWithOffset&& other) noexcept + : NumericVectorWithOffset() +{ + swap(*this, other); +} + +// assignment +template +NumericVectorWithOffset& +NumericVectorWithOffset:: +operator=(const NumericVectorWithOffset& other) +{ + base_type::operator=(other); + return *this; +} + // addition template inline NumericVectorWithOffset diff --git a/src/include/stir/VectorWithOffset.h b/src/include/stir/VectorWithOffset.h index 3f1ec8c375..4b0c04ee86 100644 --- a/src/include/stir/VectorWithOffset.h +++ b/src/include/stir/VectorWithOffset.h @@ -150,6 +150,24 @@ class VectorWithOffset //! Destructor inline virtual ~VectorWithOffset(); + //! Swap content/members of 2 objects + // implementation in .h because of templates/friends/whatever, see https://stackoverflow.com/a/61020224 + friend inline void swap(VectorWithOffset& first, VectorWithOffset& second) // nothrow + { + using std::swap; + // swap the members of two objects + swap(first.num, second.num); + swap(first.length, second.length); + swap(first.start, second.start); + swap(first.begin_allocated_memory, second.begin_allocated_memory); + swap(first.end_allocated_memory, second.end_allocated_memory); + swap(first.pointer_access, second.pointer_access); + swap(first._owns_memory_for_data, second._owns_memory_for_data); + } + + //! move constructor + /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ + VectorWithOffset(VectorWithOffset&& other) noexcept; //! change vector with new index range and point to \c data_ptr /*! \arg data_ptr should start to a contiguous block of correct size @@ -163,6 +181,7 @@ class VectorWithOffset inline void recycle(); //! assignment operator with another vector + /*! implementation avoids reallocating if sufficient memory already exists. */ inline VectorWithOffset& operator=(const VectorWithOffset& il); //! \name index range operations diff --git a/src/include/stir/VectorWithOffset.inl b/src/include/stir/VectorWithOffset.inl index bce23e1117..969d0ca5b3 100644 --- a/src/include/stir/VectorWithOffset.inl +++ b/src/include/stir/VectorWithOffset.inl @@ -315,6 +315,14 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, this->check_state(); } +template +VectorWithOffset:: +VectorWithOffset(VectorWithOffset&& other) noexcept + : VectorWithOffset() +{ + swap(*this, other); +} + template VectorWithOffset::~VectorWithOffset() { @@ -323,6 +331,17 @@ VectorWithOffset::~VectorWithOffset() _destruct_and_deallocate(); } +#if 0 +template +VectorWithOffset& +VectorWithOffset:: +operator=(VectorWithOffset other) +{ + swap(*this, other); + return *this; +} +#endif + template void VectorWithOffset::set_offset(const int min_index) From 49146cec2fb69f188015162cc7f462c268d83619 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Thu, 24 Aug 2023 11:38:44 +0100 Subject: [PATCH 07/25] Array: clean-up - make sure it works with other STIR classes, not just floats etc - re-instate VectorWithOffset constructors that take both start and end pointers for backwards compatibility --- src/include/stir/Array.inl | 3 ++- src/include/stir/VectorWithOffset.h | 23 ++++++++++++++++++++--- src/include/stir/VectorWithOffset.inl | 20 +++++--------------- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index 57390663fc..18c3e5f1d6 100644 --- a/src/include/stir/Array.inl +++ b/src/include/stir/Array.inl @@ -96,7 +96,8 @@ Array::Array(const IndexRange& range) //grow(range); this->_allocated_full_data_ptr = new elemT[range.size_all()]; // info("Array constructor range " + std::to_string(reinterpret_cast(this->_allocated_full_data_ptr)) + " of size " + std::to_string(range.size_all())); - std::fill(this->_allocated_full_data_ptr, this->_allocated_full_data_ptr + range.size_all(), elemT(0)); + // set elements to zero + std::for_each(this->_allocated_full_data_ptr, this->_allocated_full_data_ptr + range.size_all(), [](elemT& e) { assign(e, 0); }); this->init(range, this->_allocated_full_data_ptr, false); } diff --git a/src/include/stir/VectorWithOffset.h b/src/include/stir/VectorWithOffset.h index 4b0c04ee86..ea9cd9d4bc 100644 --- a/src/include/stir/VectorWithOffset.h +++ b/src/include/stir/VectorWithOffset.h @@ -139,10 +139,27 @@ class VectorWithOffset inline VectorWithOffset(const int min_index, const int max_index); //! Construct a VectorWithOffset of given length pointing to existing data - inline explicit VectorWithOffset(const int hsz, T * const data_ptr); - + inline + VectorWithOffset(const int hsz, T * const data_ptr, T * const end_of_data_ptr); + + //! Construct a VectorWithOffset of given length pointing to existing data + inline + VectorWithOffset(const int hsz, T * const data_ptr) + : VectorWithOffset(hsz, data_ptr, data_ptr + hsz) + {} + //! Construct a VectorWithOffset with offset \c min_index point to existing data - inline VectorWithOffset(const int min_index, const int max_index, T * const data_ptr); + inline + VectorWithOffset(const int min_index, const int max_index, + T * const data_ptr, + T * const end_of_data_ptr); + + //! Construct a VectorWithOffset with offset \c min_index point to existing data + inline + VectorWithOffset(const int min_index, const int max_index, + T * const data_ptr) + : VectorWithOffset(min_index, max_index, data_ptr, data_ptr + (max_index - min_index + 1)) + {} //! copy constructor inline VectorWithOffset(const VectorWithOffset& il); diff --git a/src/include/stir/VectorWithOffset.inl b/src/include/stir/VectorWithOffset.inl index 969d0ca5b3..813b59f615 100644 --- a/src/include/stir/VectorWithOffset.inl +++ b/src/include/stir/VectorWithOffset.inl @@ -290,27 +290,28 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index) } template -VectorWithOffset::VectorWithOffset(const int sz, T * const data_ptr) +VectorWithOffset::VectorWithOffset(const int sz, T * const data_ptr, T * const end_of_data_ptr) : length(static_cast(sz)), start(0), pointer_access(false), _owns_memory_for_data(false) { this->begin_allocated_memory = data_ptr; - this->end_allocated_memory = data_ptr + sz; + this->end_allocated_memory = end_of_data_ptr; this->num = this->begin_allocated_memory - this->start; this->check_state(); } template -VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, T * const data_ptr) +VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, + T * const data_ptr, T * const end_of_data_ptr) : length(static_cast(max_index - min_index) + 1), start(min_index), pointer_access(false), _owns_memory_for_data(false) { this->begin_allocated_memory = data_ptr; - this->end_allocated_memory = data_ptr + (max_index - min_index + 1); + this->end_allocated_memory = end_of_data_ptr; this->num = this->begin_allocated_memory - this->start; this->check_state(); } @@ -331,17 +332,6 @@ VectorWithOffset::~VectorWithOffset() _destruct_and_deallocate(); } -#if 0 -template -VectorWithOffset& -VectorWithOffset:: -operator=(VectorWithOffset other) -{ - swap(*this, other); - return *this; -} -#endif - template void VectorWithOffset::set_offset(const int min_index) From 86a449cf175472e274c20064f8005b7a628510d9 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Thu, 24 Aug 2023 14:24:47 +0100 Subject: [PATCH 08/25] Array: remove wrong/not implemented constructors The NumericVectorWithOffset constructor would only work when T=NUMBER. The 1D Array constructor taking an `elemT*` wasn't implemented yet. --- src/include/stir/Array.h | 3 +++ src/include/stir/NumericVectorWithOffset.h | 5 +---- src/include/stir/NumericVectorWithOffset.inl | 6 ------ src/test/test_Array.cxx | 4 ++++ 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/include/stir/Array.h b/src/include/stir/Array.h index b5387b2933..056a40a11a 100644 --- a/src/include/stir/Array.h +++ b/src/include/stir/Array.h @@ -370,6 +370,8 @@ class Array<1, elemT> : public NumericVectorWithOffset //! constructor given first and last indices, initialising elements to 0 inline Array(const int min_index, const int max_index); +#if 0 + // TODO. these are not implemented yet. //! constructor given an IndexRange<1>, pointing to \c data_ptr inline Array(const IndexRange<1>& range, elemT * const data_ptr ); @@ -378,6 +380,7 @@ class Array<1, elemT> : public NumericVectorWithOffset \arg data_ptr should start to a contiguous block of correct size */ inline Array(const int min_index, const int max_index, elemT * const data_ptr); +#endif //! constructor from basetype inline Array(const NumericVectorWithOffset& il); diff --git a/src/include/stir/NumericVectorWithOffset.h b/src/include/stir/NumericVectorWithOffset.h index 05b91a063f..f63d9fab8c 100644 --- a/src/include/stir/NumericVectorWithOffset.h +++ b/src/include/stir/NumericVectorWithOffset.h @@ -4,7 +4,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2005-06-03, Hammersmith Imanet Ltd Copyright (C) 2011-07-01 - 2012, Kris Thielemans - Copyright (C) 2020, 2023 UCL + Copyright (C) 2020, 2023 University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license @@ -59,9 +59,6 @@ class NumericVectorWithOffset : public VectorWithOffset //! Construct a NumericVectorWithOffset of elements with offset \c min_index inline NumericVectorWithOffset(const int min_index, const int max_index); - //! Construct a NumericVectorWithOffset of elements with offset \c min_index pointing to \c data_ptr - inline NumericVectorWithOffset(const int min_index, const int max_index, elemT * const data_ptr); - //! Constructor from an object of this class' base_type inline NumericVectorWithOffset(const VectorWithOffset& t); diff --git a/src/include/stir/NumericVectorWithOffset.inl b/src/include/stir/NumericVectorWithOffset.inl index 0cb5be5024..5fd744596a 100644 --- a/src/include/stir/NumericVectorWithOffset.inl +++ b/src/include/stir/NumericVectorWithOffset.inl @@ -42,12 +42,6 @@ inline NumericVectorWithOffset::NumericVectorWithOffset(const int min : base_type(min_index, max_index) {} -template -inline -NumericVectorWithOffset::NumericVectorWithOffset(const int min_index, const int max_index, NUMBER * const data_ptr) - : base_type(min_index, max_index, data_ptr) -{} - template NumericVectorWithOffset::NumericVectorWithOffset(const VectorWithOffset& t) : base_type(t) diff --git a/src/test/test_Array.cxx b/src/test/test_Array.cxx index bfde52932c..8a8ff42fe7 100644 --- a/src/test/test_Array.cxx +++ b/src/test/test_Array.cxx @@ -353,6 +353,7 @@ ArrayTests::run_tests() std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); Array<1,float> preallocated; preallocated.init(test.get_index_range(), &mem[0], false); + check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory"); check_if_equal(test, preallocated, "test preallocated: equality"); std::copy(test.begin_all_const(), test.end_all_const(), preallocated.begin_all()); check_if_equal(test, preallocated, "test preallocated: copy with full_iterator"); @@ -387,6 +388,7 @@ ArrayTests::run_tests() std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); Array<1,float> test_from_mem; test_from_mem.init(test.get_index_range(), &mem[0], true); + check(test_from_mem.owns_memory_for_data(), "test preallocated with copy: should own memory"); check_if_equal(test, test_from_mem, "test construct from mem: equality"); std::copy(test.begin_all_const(), test.end_all_const(), test_from_mem.begin_all()); check_if_equal(test, test_from_mem, "test construct from mem: copy with full_iterator"); @@ -523,6 +525,7 @@ ArrayTests::run_tests() } Array<2,float> preallocated; preallocated.init(t2.get_index_range(), &mem[0], false); + //check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory"); check_if_equal(t2, preallocated, "test preallocated: equality"); std::copy(t2.begin_all_const(), t2.end_all_const(), preallocated.begin_all()); check_if_equal(t2, preallocated, "test preallocated: copy with full_iterator"); @@ -1013,6 +1016,7 @@ ArrayTests::run_tests() Array<4,int> a1; a1.init(range, v.data(), false); t.stop(); + //check(!a1.owns_memory_for_data(), "test preallocated without copy: should not own memory"); create_duration = t.value(); std::cerr << "contiguous array creation (total) " << t.value()*1000 << "ms\n"; t.start(); From 205197c814d7fa3e319cdfb92f0051f08fd4fb8d Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Thu, 24 Aug 2023 17:18:51 +0100 Subject: [PATCH 09/25] resolve std::swap for VC Seems that VC gets confused by the new Array::swap, so add std:: explicitly --- src/buildblock/ProjDataInfoCylindrical.cxx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/buildblock/ProjDataInfoCylindrical.cxx b/src/buildblock/ProjDataInfoCylindrical.cxx index 8878214a0a..8cc5942cbc 100644 --- a/src/buildblock/ProjDataInfoCylindrical.cxx +++ b/src/buildblock/ProjDataInfoCylindrical.cxx @@ -43,7 +43,6 @@ using std::min_element; using std::max_element; using std::min; using std::max; -using std::swap; using std::endl; using std::string; using std::pair; @@ -113,7 +112,7 @@ ProjDataInfoCylindrical::ProjDataInfoCylindrical(const shared_ptr& scan min_ring_diff[segment_num], max_ring_diff[segment_num], segment_num); - swap(min_ring_diff[segment_num], max_ring_diff[segment_num]); + std::swap(min_ring_diff[segment_num], max_ring_diff[segment_num]); } } From bc5a282e6d47153e19e113e6d32090061255e399 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Sun, 4 Feb 2024 10:25:41 +0000 Subject: [PATCH 10/25] add Array constructor from ptr [ci skip] make Array:init from ptr private test_Array is crashing due to memory bug in grow --- src/include/stir/Array.h | 73 ++++++++++++++++++++++++-------------- src/include/stir/Array.inl | 25 +++++++++++-- src/test/test_Array.cxx | 15 ++++---- 3 files changed, 75 insertions(+), 38 deletions(-) diff --git a/src/include/stir/Array.h b/src/include/stir/Array.h index 056a40a11a..09b4137f7b 100644 --- a/src/include/stir/Array.h +++ b/src/include/stir/Array.h @@ -136,6 +136,20 @@ class Array : public NumericVectorWithOffset, e //! Construct an Array of given range of indices, elements are initialised to 0 inline explicit Array(const IndexRange&); + //! Construct an Array from existing contiguous data + /*! + \arg data_ptr should point to a contiguous block of correct size + \arg copy_data if \c false, the constructed Array will essentially be a "view" of the + \a data_ptr block. Therefore, any modifications to the array will modify the data at \a data_ptr. + + The C-array \data_ptr will be accessed with the last dimension running fastest + ("row-major" order). + + \warning When using \a copy_data = \c false, the user is responsible of memory management of + the block pointed to by \a data_ptr. + */ + inline Array(const IndexRange& range, elemT * const data_ptr, bool copy_data); + #ifndef SWIG // swig 2.0.4 gets confused by base_type (due to numeric template arguments) // therefore, we declare this constructor using the "self" type, @@ -170,17 +184,6 @@ class Array : public NumericVectorWithOffset, e /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ Array& operator=(Array other); - - //! change the array to a new range of indices, pointing to \c data_ptr - /*! - \arg data_ptr should point to a contiguous block of correct size - - The C-array \data_ptr will be accessed with the last dimension running fastest - ("row-major" order). - */ - inline virtual void - init(const IndexRange& range, elemT * const data_ptr, bool copy_data); - /*! @name functions returning full_iterators*/ //@{ //! start value for iterating through all elements in the array, see full_iterator @@ -313,6 +316,19 @@ class Array : public NumericVectorWithOffset, e //! A pointer to the allocated chunk if the array is constructed that way, zero otherwise elemT* _allocated_full_data_ptr; + + //! change the array to a new range of indices, pointing to \c data_ptr + /*! + \arg data_ptr should point to a contiguous block of correct size + + The C-array \data_ptr will be accessed with the last dimension running fastest + ("row-major" order). + */ + inline void + init(const IndexRange& range, elemT * const data_ptr, bool copy_data); + // Make sure that we can access init() recursively + template friend class Array; + }; /************************************************** @@ -370,17 +386,16 @@ class Array<1, elemT> : public NumericVectorWithOffset //! constructor given first and last indices, initialising elements to 0 inline Array(const int min_index, const int max_index); -#if 0 - // TODO. these are not implemented yet. - //! constructor given an IndexRange<1>, pointing to \c data_ptr - inline Array(const IndexRange<1>& range, elemT * const data_ptr ); - - //! constructor given first and last indices, pointing to \c data_ptr + //! constructor given an IndexRange<1>, pointing to existing contiguous data /*! - \arg data_ptr should start to a contiguous block of correct size + \arg data_ptr should point to a contiguous block of correct size + \arg copy_data if \c false, the constructed Array will essentially be a "view" of the + \a data_ptr block. Therefore, any modifications to the array will modify the data at \a data_ptr. + + \warning When using \a copy_data = \c false, the user is responsible of memory management of + the block pointed to by \a data_ptr. */ - inline Array(const int min_index, const int max_index, elemT * const data_ptr); -#endif + inline Array(const IndexRange<1>& range, elemT * const data_ptr, bool copy_data); //! constructor from basetype inline Array(const NumericVectorWithOffset& il); @@ -407,13 +422,6 @@ class Array<1, elemT> : public NumericVectorWithOffset //! assignment inline Array& operator=(const Array& other); - - //! change vector with new index range and point to \c data_ptr - /*! - \arg data_ptr should start to a contiguous block of correct size - */ - inline void init(const IndexRange<1>& range, elemT * const data_ptr, bool copy_data); - /*! @name functions returning full_iterators*/ //@{ //! start value for iterating through all elements in the array, see full_iterator @@ -542,6 +550,17 @@ class Array<1, elemT> : public NumericVectorWithOffset inline const elemT& at(const BasicCoordinate<1, int>& c) const; //@} + + private: + // Make sure we can call init() recursively. + template friend class Array; + + //! change vector with new index range and point to \c data_ptr + /*! + \arg data_ptr should start to a contiguous block of correct size + */ + inline void init(const IndexRange<1>& range, elemT * const data_ptr, bool copy_data); + }; END_NAMESPACE_STIR diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index 18c3e5f1d6..7887cab837 100644 --- a/src/include/stir/Array.inl +++ b/src/include/stir/Array.inl @@ -101,11 +101,24 @@ Array::Array(const IndexRange& range) this->init(range, this->_allocated_full_data_ptr, false); } +template +Array::Array(const IndexRange& range, elemT * const data_ptr, bool copy_data) +{ + if (copy_data) + { + this->_allocated_full_data_ptr = new elemT[range.size_all()]; + std::copy(data_ptr, data_ptr + range.size_all(), this->_allocated_full_data_ptr); + } + else + this->_allocated_full_data_ptr = data_ptr; + this->init(range, this->_allocated_full_data_ptr, false); +} + template Array::Array(const self& t) : base_type(t), _allocated_full_data_ptr(0) { - // info("constructor copy of size " + std::to_string(this->size_all())); + // info("constructor " + std::to_string(num_dimensions) + "copy of size " + std::to_string(this->size_all())); } #ifndef SWIG @@ -114,7 +127,7 @@ template Array::Array(const base_type& t) : base_type(t), _allocated_full_data_ptr(0) { - // info("constructor basetype of size " + std::to_string(this->size_all())); + // info("constructor basetype " + std::to_string(num_dimensions) + " of size " + std::to_string(this->size_all())); } #endif @@ -133,6 +146,7 @@ Array::Array(Array&& other) noexce : Array() { swap(*this, other); + // info("move constructor " + std::to_string(num_dimensions) + "copy of size " + std::to_string(this->size_all())); } template @@ -141,6 +155,7 @@ Array:: operator=(Array other) { swap(*this, other); + // info("Array= " + std::to_string(num_dimensions) + "copy of size " + std::to_string(this->size_all())); return *this; } @@ -601,6 +616,12 @@ Array<1, elemT>::Array(const int min_index, const int max_index) grow(min_index, max_index); } +template +Array<1, elemT>::Array(const IndexRange<1>& range, elemT * const data_ptr, bool copy_data) +{ + this->init(range, data_ptr, copy_data); +} + template Array<1, elemT>::Array(const base_type& il) : base_type(il) diff --git a/src/test/test_Array.cxx b/src/test/test_Array.cxx index 8a8ff42fe7..d80812d16b 100644 --- a/src/test/test_Array.cxx +++ b/src/test/test_Array.cxx @@ -351,8 +351,7 @@ ArrayTests::run_tests() { std::vector mem(test.get_index_range().size_all()); std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); - Array<1,float> preallocated; - preallocated.init(test.get_index_range(), &mem[0], false); + Array<1,float> preallocated(test.get_index_range(), &mem[0], false); check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory"); check_if_equal(test, preallocated, "test preallocated: equality"); std::copy(test.begin_all_const(), test.end_all_const(), preallocated.begin_all()); @@ -386,8 +385,7 @@ ArrayTests::run_tests() { std::vector mem(test.get_index_range().size_all()); std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); - Array<1,float> test_from_mem; - test_from_mem.init(test.get_index_range(), &mem[0], true); + Array<1,float> test_from_mem(test.get_index_range(), &mem[0], true); check(test_from_mem.owns_memory_for_data(), "test preallocated with copy: should own memory"); check_if_equal(test, test_from_mem, "test construct from mem: equality"); std::copy(test.begin_all_const(), test.end_all_const(), test_from_mem.begin_all()); @@ -505,7 +503,8 @@ ArrayTests::run_tests() { // copy constructor; Array<2, float> t21(t2); - check_if_equal(t21, t2, "test Array2D copy constructor"); + check_if_equal( t21[3][2] , 6.F, "test Array2D copy consructor element"); + check_if_equal(t21 , t2, "test Array2D copy constructor all elements" ); // 'assignment constructor' (this simply calls copy constructor) Array<2, float> t22 = t2; check_if_equal(t22, t2, "test Array2D copy constructor"); @@ -523,8 +522,7 @@ ArrayTests::run_tests() 2 - t2[first_min_idx].get_min_index()], "check row-major order in 2D"); } - Array<2,float> preallocated; - preallocated.init(t2.get_index_range(), &mem[0], false); + Array<2,float> preallocated(t2.get_index_range(), &mem[0], false); //check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory"); check_if_equal(t2, preallocated, "test preallocated: equality"); std::copy(t2.begin_all_const(), t2.end_all_const(), preallocated.begin_all()); @@ -1013,8 +1011,7 @@ ArrayTests::run_tests() t.stop(); std::cerr << "vector creation " << t.value()*1000 << "ms\n"; t.start(); - Array<4,int> a1; - a1.init(range, v.data(), false); + Array<4,int> a1(range, v.data(), false); t.stop(); //check(!a1.owns_memory_for_data(), "test preallocated without copy: should not own memory"); create_duration = t.value(); From a399a9621406ff4001d6fd2220877f54fd0b8138 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Sun, 11 Feb 2024 20:10:07 +0000 Subject: [PATCH 11/25] Array: first version using shared_ptr (WIP) nD Array's now store the "full" memory via shared_ptr. This automates memory management. 1D Array's are still TODO. test_Array now works ok, except for 1D arrays. Some testing code remains in test_Array --- src/include/stir/Array.h | 6 ++--- src/include/stir/Array.inl | 32 +++++++++++------------ src/include/stir/shared_ptr.h | 9 +++++++ src/test/test_Array.cxx | 48 ++++++++++++++++++++++++++++++++--- 4 files changed, 72 insertions(+), 23 deletions(-) diff --git a/src/include/stir/Array.h b/src/include/stir/Array.h index 09b4137f7b..72f0681e5d 100644 --- a/src/include/stir/Array.h +++ b/src/include/stir/Array.h @@ -148,7 +148,7 @@ class Array : public NumericVectorWithOffset, e \warning When using \a copy_data = \c false, the user is responsible of memory management of the block pointed to by \a data_ptr. */ - inline Array(const IndexRange& range, elemT * const data_ptr, bool copy_data); + inline Array(const IndexRange& range, shared_ptr data_sptr, bool copy_data); #ifndef SWIG // swig 2.0.4 gets confused by base_type (due to numeric template arguments) @@ -315,7 +315,7 @@ class Array : public NumericVectorWithOffset, e mutable bool _full_pointer_access; //! A pointer to the allocated chunk if the array is constructed that way, zero otherwise - elemT* _allocated_full_data_ptr; + shared_ptr _allocated_full_data_ptr; //! change the array to a new range of indices, pointing to \c data_ptr /*! @@ -395,7 +395,7 @@ class Array<1, elemT> : public NumericVectorWithOffset \warning When using \a copy_data = \c false, the user is responsible of memory management of the block pointed to by \a data_ptr. */ - inline Array(const IndexRange<1>& range, elemT * const data_ptr, bool copy_data); + inline Array(const IndexRange<1>& range, shared_ptr data_sptr, bool copy_data); //! constructor from basetype inline Array(const NumericVectorWithOffset& il); diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index 7887cab837..5f56ecd26d 100644 --- a/src/include/stir/Array.inl +++ b/src/include/stir/Array.inl @@ -86,37 +86,35 @@ Array::grow(const IndexRange& range) template Array::Array() - : base_type(), _allocated_full_data_ptr(0) + : base_type(), _allocated_full_data_ptr(nullptr) {} template Array::Array(const IndexRange& range) - : base_type(), _allocated_full_data_ptr(0) + : base_type(), _allocated_full_data_ptr(new elemT[range.size_all()]) { - //grow(range); - this->_allocated_full_data_ptr = new elemT[range.size_all()]; // info("Array constructor range " + std::to_string(reinterpret_cast(this->_allocated_full_data_ptr)) + " of size " + std::to_string(range.size_all())); // set elements to zero - std::for_each(this->_allocated_full_data_ptr, this->_allocated_full_data_ptr + range.size_all(), [](elemT& e) { assign(e, 0); }); - this->init(range, this->_allocated_full_data_ptr, false); + std::for_each(this->_allocated_full_data_ptr.get(), this->_allocated_full_data_ptr.get() + range.size_all(), [](elemT& e) { assign(e, 0); }); + this->init(range, this->_allocated_full_data_ptr.get(), false); } template -Array::Array(const IndexRange& range, elemT * const data_ptr, bool copy_data) + Array::Array(const IndexRange& range, shared_ptr data_sptr, bool copy_data) { if (copy_data) { - this->_allocated_full_data_ptr = new elemT[range.size_all()]; - std::copy(data_ptr, data_ptr + range.size_all(), this->_allocated_full_data_ptr); + this->_allocated_full_data_ptr = std::make_shared(range.size_all()); + std::copy(data_sptr.get(), data_sptr.get() + range.size_all(), this->_allocated_full_data_ptr.get()); } else - this->_allocated_full_data_ptr = data_ptr; - this->init(range, this->_allocated_full_data_ptr, false); + this->_allocated_full_data_ptr = data_sptr; + this->init(range, this->_allocated_full_data_ptr.get(), false); } template Array::Array(const self& t) -: base_type(t), _allocated_full_data_ptr(0) +: base_type(t), _allocated_full_data_ptr(nullptr) { // info("constructor " + std::to_string(num_dimensions) + "copy of size " + std::to_string(this->size_all())); } @@ -125,7 +123,7 @@ Array::Array(const self& t) // swig cannot parse this ATM, but we don't need it anyway in the wrappers template Array::Array(const base_type& t) -: base_type(t), _allocated_full_data_ptr(0) +: base_type(t), _allocated_full_data_ptr(nullptr) { // info("constructor basetype " + std::to_string(num_dimensions) + " of size " + std::to_string(this->size_all())); } @@ -137,7 +135,7 @@ Array::~Array() if (this->_allocated_full_data_ptr) { // info("Array destructor full_data_ptr " + std::to_string(reinterpret_cast(this->_allocated_full_data_ptr)) + " of size " + std::to_string(this->size_all())); - delete [] this->_allocated_full_data_ptr; + // delete [] this->_allocated_full_data_ptr; } } @@ -617,9 +615,11 @@ Array<1, elemT>::Array(const int min_index, const int max_index) } template -Array<1, elemT>::Array(const IndexRange<1>& range, elemT * const data_ptr, bool copy_data) +Array<1, elemT>::Array(const IndexRange<1>& range, shared_ptr data_sptr, bool copy_data) { - this->init(range, data_ptr, copy_data); + if (!copy_data) + error("1D Array !copy TODO"); + this->init(range, data_sptr.get(), copy_data); } template diff --git a/src/include/stir/shared_ptr.h b/src/include/stir/shared_ptr.h index 1b7fd14ff0..dd55747606 100644 --- a/src/include/stir/shared_ptr.h +++ b/src/include/stir/shared_ptr.h @@ -35,10 +35,19 @@ using boost::static_pointer_cast; # define MAKE_SHARED boost::make_shared } // namespace stir #else +#if __cplusplus < 201703L +#include +namespace stir { + using std::experimental::shared_ptr; +} +#else # include namespace stir { using std::shared_ptr; +} +#endif +namespace stir { using std::dynamic_pointer_cast; using std::static_pointer_cast; //! work-around for using std::make_shared on old compilers diff --git a/src/test/test_Array.cxx b/src/test/test_Array.cxx index d80812d16b..9353bf3f22 100644 --- a/src/test/test_Array.cxx +++ b/src/test/test_Array.cxx @@ -47,6 +47,8 @@ // for open_read/write_binary #include "stir/utilities.h" +#include "stir/info.h" +#include "stir/error.h" #include "stir/HighResWallClockTimer.h" @@ -277,6 +279,31 @@ class ArrayTests : public RunTests void run_tests() override; }; +template +class Deleter +{ +public: + Deleter(std::vector& v) : + v(v) + {} + void operator()(T* vptr) + { + info("in deleter"); + if (vptr != v.data()) + error("deleter error"); + //~v; + } +private: + std::vector& v; +}; + +template +shared_ptr vec_to_shared(std::vector& v) +{ + shared_ptr sptr(v.data(), Deleter(v)); + return sptr; +} + void ArrayTests::run_tests() { @@ -348,10 +375,16 @@ ArrayTests::run_tests() } // using preallocated memory +#if 0 { std::vector mem(test.get_index_range().size_all()); std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); - Array<1,float> preallocated(test.get_index_range(), &mem[0], false); + //Array<1,float> preallocated(test.get_index_range(), &mem[0], false); + Array<1,float> preallocated(test.get_index_range(), vec_to_shared(mem), false); + //shared_ptr mem_sptr(new float [test.get_index_range().size_all()]); + //auto mem = mem_sptr.get(); + //std::copy(test.begin_all_const(), test.end_all_const(), mem); + //Array<1,float> preallocated(test.get_index_range(), mem_sptr, false); check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory"); check_if_equal(test, preallocated, "test preallocated: equality"); std::copy(test.begin_all_const(), test.end_all_const(), preallocated.begin_all()); @@ -381,11 +414,16 @@ ArrayTests::run_tests() check_if_equal(preallocated[min+1], 12345.F, "test preallocated: grow uses different memory"); } } +#endif // copying from existing memory { std::vector mem(test.get_index_range().size_all()); std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); - Array<1,float> test_from_mem(test.get_index_range(), &mem[0], true); + //Array<1,float> test_from_mem(test.get_index_range(), &mem[0], true); + Array<1,float> test_from_mem(test.get_index_range(), vec_to_shared(mem), true); + //shared_ptr mem(test.get_index_range().size_all()); + //std::copy(test.begin_all_const(), test.end_all_const(), mem.get()); + //Array<1,float> preallocated(test.get_index_range(), mem.get(), false); check(test_from_mem.owns_memory_for_data(), "test preallocated with copy: should own memory"); check_if_equal(test, test_from_mem, "test construct from mem: equality"); std::copy(test.begin_all_const(), test.end_all_const(), test_from_mem.begin_all()); @@ -522,7 +560,8 @@ ArrayTests::run_tests() 2 - t2[first_min_idx].get_min_index()], "check row-major order in 2D"); } - Array<2,float> preallocated(t2.get_index_range(), &mem[0], false); + //Array<2,float> preallocated(t2.get_index_range(), &mem[0], false); + Array<2,float> preallocated(t2.get_index_range(), vec_to_shared(mem), false); //check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory"); check_if_equal(t2, preallocated, "test preallocated: equality"); std::copy(t2.begin_all_const(), t2.end_all_const(), preallocated.begin_all()); @@ -1011,7 +1050,8 @@ ArrayTests::run_tests() t.stop(); std::cerr << "vector creation " << t.value()*1000 << "ms\n"; t.start(); - Array<4,int> a1(range, v.data(), false); + //Array<4,int> a1(range, v.data(), false); + Array<4,int> a1(range, vec_to_shared(v), false); t.stop(); //check(!a1.owns_memory_for_data(), "test preallocated without copy: should not own memory"); create_duration = t.value(); From 12cec9a97cdb5caaca9bbccb9926e14fdc60ca52 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Sun, 11 Feb 2024 21:22:05 +0000 Subject: [PATCH 12/25] require C++-17 needed for shared_ptr, unless we'd implement deleters ourselves --- .github/workflows/build-test.yml | 2 +- CMakeLists.txt | 2 +- src/include/stir/Array.inl | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index c7d1576e0b..a39f592829 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -79,7 +79,7 @@ jobs: compiler: gcc compiler_version: 12 cuda_version: "12.1.0" - BUILD_FLAGS: "-DSTIR_OPENMP=ON -DCMAKE_CXX_STANDARD=14" + BUILD_FLAGS: "-DSTIR_OPENMP=ON -DCMAKE_CXX_STANDARD=17" BUILD_TYPE: "Release" parallelproj: "ON" ROOT: "OFF" diff --git a/CMakeLists.txt b/CMakeLists.txt index 2511aa596f..7343db929e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -33,7 +33,7 @@ include(src/cmake/SetC++Version.cmake) # minimum C++ version required (you can still ask for a more recent one # by setting CMAKE_CXX_STANDARD) -UseCXX(14) +UseCXX(17) # set default build-type to Release if(NOT CMAKE_BUILD_TYPE) diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index 5f56ecd26d..388800eff6 100644 --- a/src/include/stir/Array.inl +++ b/src/include/stir/Array.inl @@ -104,7 +104,7 @@ template { if (copy_data) { - this->_allocated_full_data_ptr = std::make_shared(range.size_all()); + this->_allocated_full_data_ptr = std::shared_ptr(new elemT[range.size_all()]); std::copy(data_sptr.get(), data_sptr.get() + range.size_all(), this->_allocated_full_data_ptr.get()); } else From 3c29a2a6f042ed3ed4b0b263a0861810954dc979 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Sun, 11 Feb 2024 23:57:09 +0000 Subject: [PATCH 13/25] Array: use shared_ptr for VectorWithOffset all Array tests pass --- src/include/stir/Array.inl | 2 +- src/include/stir/VectorWithOffset.h | 16 +++---- src/include/stir/VectorWithOffset.inl | 60 ++++++++++----------------- src/test/test_Array.cxx | 3 +- 4 files changed, 34 insertions(+), 47 deletions(-) diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index 388800eff6..c4b5179191 100644 --- a/src/include/stir/Array.inl +++ b/src/include/stir/Array.inl @@ -618,7 +618,7 @@ template Array<1, elemT>::Array(const IndexRange<1>& range, shared_ptr data_sptr, bool copy_data) { if (!copy_data) - error("1D Array !copy TODO"); + this->allocated_memory_sptr = data_sptr; this->init(range, data_sptr.get(), copy_data); } diff --git a/src/include/stir/VectorWithOffset.h b/src/include/stir/VectorWithOffset.h index ea9cd9d4bc..858c9dfb6d 100644 --- a/src/include/stir/VectorWithOffset.h +++ b/src/include/stir/VectorWithOffset.h @@ -23,7 +23,7 @@ */ -#include "stir/common.h" +#include "stir/shared_ptr.h" #include "boost/iterator/iterator_adaptor.hpp" #include "boost/iterator/reverse_iterator.hpp" @@ -180,16 +180,12 @@ class VectorWithOffset swap(first.end_allocated_memory, second.end_allocated_memory); swap(first.pointer_access, second.pointer_access); swap(first._owns_memory_for_data, second._owns_memory_for_data); + swap(first.allocated_memory_sptr, second.allocated_memory_sptr); } //! move constructor /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ VectorWithOffset(VectorWithOffset&& other) noexcept; - //! change vector with new index range and point to \c data_ptr - /*! - \arg data_ptr should start to a contiguous block of correct size - */ - inline void init(const int min_index, const int max_index, T * const data_ptr, bool copy_data); //! Free all memory and make object as if default-constructed /*! This is not the same as resize(0), as the latter does not @@ -403,10 +399,16 @@ class VectorWithOffset protected: //! pointer to (*this)[0] (taking get_min_index() into account that is). - T* num; + T* num; // TODOKT make private + shared_ptr allocated_memory_sptr; //! Called internally to see if all variables are consistent inline void check_state() const; + //! change vector with new index range and point to \c data_ptr + /*! + \arg data_ptr should start to a contiguous block of correct size + */ + inline void init(const int min_index, const int max_index, T* const data_ptr, bool copy_data); private: //! length of vector diff --git a/src/include/stir/VectorWithOffset.inl b/src/include/stir/VectorWithOffset.inl index 813b59f615..796cd67815 100644 --- a/src/include/stir/VectorWithOffset.inl +++ b/src/include/stir/VectorWithOffset.inl @@ -36,9 +36,10 @@ VectorWithOffset::init() { length = 0; // i.e. an empty row of zero length, start = 0; // no offsets - num = 0; // and no data. - begin_allocated_memory = 0; - end_allocated_memory = 0; + num = nullptr; // and no data. + begin_allocated_memory = nullptr; + end_allocated_memory = nullptr; + allocated_memory_sptr = nullptr; } template @@ -86,6 +87,10 @@ VectorWithOffset::check_state() const assert(begin_allocated_memory <= num + start); assert(end_allocated_memory >= begin_allocated_memory); assert(static_cast(end_allocated_memory - begin_allocated_memory) >= length); + if (allocated_memory_sptr) + { + // assert(_owns_memory_for_data); + } } template @@ -102,7 +107,7 @@ VectorWithOffset::_destruct_and_deallocate() // as begin_allocated_memory is == 0 in that case, and delete[] 0 doesn't do anything // (I think). Anyway, we're on the safe side now... if (this->owns_memory_for_data() && this->capacity() != 0) - delete[] this->begin_allocated_memory; + this->allocated_memory_sptr = nullptr; } template @@ -254,21 +259,8 @@ VectorWithOffset::VectorWithOffset() template VectorWithOffset::VectorWithOffset(const int hsz) - : length(hsz), - start(0), - pointer_access(false), - _owns_memory_for_data(true) -{ - if ((hsz > 0)) - { - num = new T[hsz]; - begin_allocated_memory = num; - end_allocated_memory = num + length; - } - else - this->init(); - this->check_state(); -} + : VectorWithOffset(0, hsz - 1) +{} template VectorWithOffset::VectorWithOffset(const int min_index, const int max_index) @@ -279,10 +271,10 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index) { if (max_index >= min_index) { - num = new T[length]; - begin_allocated_memory = num; - end_allocated_memory = num + length; - num -= min_index; + allocated_memory_sptr = shared_ptr(new T[length]); + begin_allocated_memory = allocated_memory_sptr.get(); + end_allocated_memory = begin_allocated_memory + length; + num = begin_allocated_memory - min_index; } else this->init(); @@ -291,16 +283,8 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index) template VectorWithOffset::VectorWithOffset(const int sz, T * const data_ptr, T * const end_of_data_ptr) - : length(static_cast(sz)), - start(0), - pointer_access(false), - _owns_memory_for_data(false) -{ - this->begin_allocated_memory = data_ptr; - this->end_allocated_memory = end_of_data_ptr; - this->num = this->begin_allocated_memory - this->start; - this->check_state(); -} + : VectorWithOffset(0, sz - 1, data_ptr, end_of_data_ptr) +{} template VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, @@ -308,7 +292,8 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, : length(static_cast(max_index - min_index) + 1), start(min_index), pointer_access(false), - _owns_memory_for_data(false) + _owns_memory_for_data(false), + allocated_memory_sptr(nullptr) { this->begin_allocated_memory = data_ptr; this->end_allocated_memory = end_of_data_ptr; @@ -397,11 +382,12 @@ VectorWithOffset::reserve(const int new_capacity_min_index, const int new_cap // check if data is being accessed via a pointer (see get_data_ptr()) assert(pointer_access == false); // TODO use allocator here instead of new - T* newmem = new T[new_capacity]; + shared_ptr new_allocated_memory_sptr(new T[new_capacity]); const unsigned extra_at_the_left = length == 0 ? 0U : std::max(0, this->get_min_index() - actual_capacity_min_index); - std::copy(this->begin(), this->end(), newmem + extra_at_the_left); + std::copy(this->begin(), this->end(), new_allocated_memory_sptr.get() + extra_at_the_left); this->_destruct_and_deallocate(); - begin_allocated_memory = newmem; + allocated_memory_sptr = std::move(new_allocated_memory_sptr); + begin_allocated_memory = allocated_memory_sptr.get(); end_allocated_memory = begin_allocated_memory + new_capacity; _owns_memory_for_data = true; num = begin_allocated_memory + extra_at_the_left - (length > 0 ? start : 0); diff --git a/src/test/test_Array.cxx b/src/test/test_Array.cxx index 9353bf3f22..d5ed8c1c47 100644 --- a/src/test/test_Array.cxx +++ b/src/test/test_Array.cxx @@ -375,7 +375,6 @@ ArrayTests::run_tests() } // using preallocated memory -#if 0 { std::vector mem(test.get_index_range().size_all()); std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); @@ -414,7 +413,7 @@ ArrayTests::run_tests() check_if_equal(preallocated[min+1], 12345.F, "test preallocated: grow uses different memory"); } } -#endif + // copying from existing memory { std::vector mem(test.get_index_range().size_all()); From f4b4db826bbe185a03392866e447f97648d50aad Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Mon, 12 Feb 2024 00:00:21 +0000 Subject: [PATCH 14/25] [SWIG] ignore swap and *full_data_ptr --- src/swig/stir.i | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/swig/stir.i b/src/swig/stir.i index 9ac439079e..437e994da7 100644 --- a/src/swig/stir.i +++ b/src/swig/stir.i @@ -576,6 +576,7 @@ %newobject *::get_empty_copy; %newobject *::read_from_file; +%ignore **::swap; %ignore *::ask_parameters; %ignore *::create_shared_clone; %ignore *::read_from_stream; @@ -583,6 +584,10 @@ %ignore *::get_const_data_ptr; %ignore *::release_data_ptr; %ignore *::release_const_data_ptr; +%ignore *::get_full_data_ptr; +%ignore *::get_const_full_data_ptr; +%ignore *::release_full_data_ptr; +%ignore *::release_const_full_data_ptr; #if defined(SWIGPYTHON) %rename(__assign__) *::operator=; From 2ba9924e8767419f91cf9300d1b3ca026d57f9b4 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Mon, 12 Feb 2024 00:34:47 +0000 Subject: [PATCH 15/25] Array: remove private member _owns_memory_for_data no longer needed as we're using a shared and can test that. --- src/include/stir/VectorWithOffset.h | 4 +--- src/include/stir/VectorWithOffset.inl | 23 +++++------------------ src/test/test_Array.cxx | 1 - 3 files changed, 6 insertions(+), 22 deletions(-) diff --git a/src/include/stir/VectorWithOffset.h b/src/include/stir/VectorWithOffset.h index 858c9dfb6d..2ccddc7a57 100644 --- a/src/include/stir/VectorWithOffset.h +++ b/src/include/stir/VectorWithOffset.h @@ -179,7 +179,6 @@ class VectorWithOffset swap(first.begin_allocated_memory, second.begin_allocated_memory); swap(first.end_allocated_memory, second.end_allocated_memory); swap(first.pointer_access, second.pointer_access); - swap(first._owns_memory_for_data, second._owns_memory_for_data); swap(first.allocated_memory_sptr, second.allocated_memory_sptr); } @@ -234,7 +233,7 @@ class VectorWithOffset //! change the range of the vector, new elements are set to \c T() /*! New memory is allocated if the range grows outside the range specified by get_capacity_min_index() till get_capacity_max_index(). Data is then copied - and old memory deallocated (unless owns_memory_for_data() is false). + and old memory deallocated (unless it is shared). \todo in principle reallocation could be avoided when the new range would fit in the old one by shifting. @@ -428,7 +427,6 @@ class VectorWithOffset //! boolean to test if get_data_ptr is called // This variable is declared mutable such that get_const_data_ptr() can change it. mutable bool pointer_access; - bool _owns_memory_for_data; }; END_NAMESPACE_STIR diff --git a/src/include/stir/VectorWithOffset.inl b/src/include/stir/VectorWithOffset.inl index 796cd67815..e71187f866 100644 --- a/src/include/stir/VectorWithOffset.inl +++ b/src/include/stir/VectorWithOffset.inl @@ -47,7 +47,6 @@ void VectorWithOffset::init(const int min_index, const int max_index, T * const data_ptr, bool copy_data) { this->pointer_access = false; - this->_owns_memory_for_data = copy_data; if (copy_data) { this->resize(min_index, max_index); @@ -68,7 +67,7 @@ template bool VectorWithOffset::owns_memory_for_data() const { - return this->_owns_memory_for_data; + return this->allocated_memory_sptr ? true : false; } /*! @@ -87,10 +86,7 @@ VectorWithOffset::check_state() const assert(begin_allocated_memory <= num + start); assert(end_allocated_memory >= begin_allocated_memory); assert(static_cast(end_allocated_memory - begin_allocated_memory) >= length); - if (allocated_memory_sptr) - { - // assert(_owns_memory_for_data); - } + assert(!allocated_memory_sptr || (allocated_memory_sptr.get() == begin_allocated_memory)); } template @@ -103,10 +99,6 @@ VectorWithOffset::_destruct_and_deallocate() // we'll have to be careful to delete only initialised elements // and just de-allocate the rest - // Check on capacity probably not really necessary - // as begin_allocated_memory is == 0 in that case, and delete[] 0 doesn't do anything - // (I think). Anyway, we're on the safe side now... - if (this->owns_memory_for_data() && this->capacity() != 0) this->allocated_memory_sptr = nullptr; } @@ -251,9 +243,8 @@ VectorWithOffset::rend() const template VectorWithOffset::VectorWithOffset() - : _owns_memory_for_data(true) + : pointer_access(false) { - pointer_access = false; this->init(); } @@ -266,8 +257,7 @@ template VectorWithOffset::VectorWithOffset(const int min_index, const int max_index) : length(static_cast(max_index - min_index) + 1), start(min_index), - pointer_access(false), - _owns_memory_for_data(true) + pointer_access(false) { if (max_index >= min_index) { @@ -292,7 +282,6 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, : length(static_cast(max_index - min_index) + 1), start(min_index), pointer_access(false), - _owns_memory_for_data(false), allocated_memory_sptr(nullptr) { this->begin_allocated_memory = data_ptr; @@ -389,7 +378,6 @@ VectorWithOffset::reserve(const int new_capacity_min_index, const int new_cap allocated_memory_sptr = std::move(new_allocated_memory_sptr); begin_allocated_memory = allocated_memory_sptr.get(); end_allocated_memory = begin_allocated_memory + new_capacity; - _owns_memory_for_data = true; num = begin_allocated_memory + extra_at_the_left - (length > 0 ? start : 0); this->check_state(); } @@ -514,8 +502,7 @@ VectorWithOffset::operator=(const VectorWithOffset& il) template VectorWithOffset::VectorWithOffset(const VectorWithOffset& il) - : pointer_access(false), - _owns_memory_for_data(true) + : pointer_access(false) { this->init(); *this = il; // Uses assignment operator (above) diff --git a/src/test/test_Array.cxx b/src/test/test_Array.cxx index d5ed8c1c47..6d00641590 100644 --- a/src/test/test_Array.cxx +++ b/src/test/test_Array.cxx @@ -384,7 +384,6 @@ ArrayTests::run_tests() //auto mem = mem_sptr.get(); //std::copy(test.begin_all_const(), test.end_all_const(), mem); //Array<1,float> preallocated(test.get_index_range(), mem_sptr, false); - check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory"); check_if_equal(test, preallocated, "test preallocated: equality"); std::copy(test.begin_all_const(), test.end_all_const(), preallocated.begin_all()); check_if_equal(test, preallocated, "test preallocated: copy with full_iterator"); From 433bd826f5335b076f9a308797bbab1cc01b3826 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Mon, 12 Feb 2024 02:10:02 +0000 Subject: [PATCH 16/25] Array: clean-up constructors that take existing data - get rid of copy_data argument for constructors but use sensible defaults - add VectorWithOffset::init such that Array doesn't need to know much --- src/include/stir/Array.h | 32 +++++----- src/include/stir/Array.inl | 24 +++----- src/include/stir/NumericVectorWithOffset.h | 13 +--- src/include/stir/NumericVectorWithOffset.inl | 15 ----- src/include/stir/VectorWithOffset.h | 64 +++++++++++++------- src/include/stir/VectorWithOffset.inl | 40 +++++++++--- src/test/test_Array.cxx | 36 +++-------- 7 files changed, 111 insertions(+), 113 deletions(-) diff --git a/src/include/stir/Array.h b/src/include/stir/Array.h index 72f0681e5d..51971c47fe 100644 --- a/src/include/stir/Array.h +++ b/src/include/stir/Array.h @@ -3,7 +3,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2011-10-14, Hammersmith Imanet Ltd Copyright (C) 2011-07-01 - 2012, Kris Thielemans - Copyright (C) 2023, University College London + Copyright (C) 2023 - 2024, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license @@ -136,19 +136,17 @@ class Array : public NumericVectorWithOffset, e //! Construct an Array of given range of indices, elements are initialised to 0 inline explicit Array(const IndexRange&); - //! Construct an Array from existing contiguous data + //! Construct an Array pointing to existing contiguous data /*! - \arg data_ptr should point to a contiguous block of correct size - \arg copy_data if \c false, the constructed Array will essentially be a "view" of the - \a data_ptr block. Therefore, any modifications to the array will modify the data at \a data_ptr. + \arg data_sptr should point to a contiguous block of correct size. + The constructed Array will essentially be a "view" of the + \c data_sptr.get() block. Therefore, any modifications to the array will modify the data at \c data_sptr.get(). + This will be true until the Array is resized. The C-array \data_ptr will be accessed with the last dimension running fastest ("row-major" order). - - \warning When using \a copy_data = \c false, the user is responsible of memory management of - the block pointed to by \a data_ptr. */ - inline Array(const IndexRange& range, shared_ptr data_sptr, bool copy_data); + inline Array(const IndexRange& range, shared_ptr data_sptr); #ifndef SWIG // swig 2.0.4 gets confused by base_type (due to numeric template arguments) @@ -388,14 +386,18 @@ class Array<1, elemT> : public NumericVectorWithOffset //! constructor given an IndexRange<1>, pointing to existing contiguous data /*! - \arg data_ptr should point to a contiguous block of correct size - \arg copy_data if \c false, the constructed Array will essentially be a "view" of the - \a data_ptr block. Therefore, any modifications to the array will modify the data at \a data_ptr. + \arg data_ptr should point to a contiguous block of correct size. + The constructed Array will essentially be a "view" of the + \c data_sptr block. Therefore, any modifications to the array will modify the data at \a data_sptr. + This will be the case until the Array is resized. + */ + inline Array(const IndexRange<1>& range, shared_ptr data_sptr); - \warning When using \a copy_data = \c false, the user is responsible of memory management of - the block pointed to by \a data_ptr. + //! constructor given an IndexRange<1> from existing contiguous data (will copy) + /*! + \arg data_ptr should point to a contiguous block of correct size. */ - inline Array(const IndexRange<1>& range, shared_ptr data_sptr, bool copy_data); + inline Array(const IndexRange<1>& range, const elemT* const data_ptr); //! constructor from basetype inline Array(const NumericVectorWithOffset& il); diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index c4b5179191..8342dfea49 100644 --- a/src/include/stir/Array.inl +++ b/src/include/stir/Array.inl @@ -4,7 +4,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2011-01-11, Hammersmith Imanet Ltd Copyright (C) 2011-07-01 - 2012, Kris Thielemans - Copyright (C) 2023, University College London + Copyright (C) 2023 - 2024, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license @@ -100,14 +100,8 @@ Array::Array(const IndexRange& range) } template - Array::Array(const IndexRange& range, shared_ptr data_sptr, bool copy_data) +Array::Array(const IndexRange& range, shared_ptr data_sptr) { - if (copy_data) - { - this->_allocated_full_data_ptr = std::shared_ptr(new elemT[range.size_all()]); - std::copy(data_sptr.get(), data_sptr.get() + range.size_all(), this->_allocated_full_data_ptr.get()); - } - else this->_allocated_full_data_ptr = data_sptr; this->init(range, this->_allocated_full_data_ptr.get(), false); } @@ -615,12 +609,14 @@ Array<1, elemT>::Array(const int min_index, const int max_index) } template -Array<1, elemT>::Array(const IndexRange<1>& range, shared_ptr data_sptr, bool copy_data) -{ - if (!copy_data) - this->allocated_memory_sptr = data_sptr; - this->init(range, data_sptr.get(), copy_data); -} +Array<1, elemT>::Array(const IndexRange<1>& range, shared_ptr data_sptr) + : base_type(range.get_min_index(), range.get_max_index(), data_sptr) +{} + +template +Array<1, elemT>::Array(const IndexRange<1>& range, const elemT* const data_ptr) + : base_type(range.get_min_index(), range.get_max_index(), data_ptr) +{} template Array<1, elemT>::Array(const base_type& il) diff --git a/src/include/stir/NumericVectorWithOffset.h b/src/include/stir/NumericVectorWithOffset.h index f63d9fab8c..fae4620f0c 100644 --- a/src/include/stir/NumericVectorWithOffset.h +++ b/src/include/stir/NumericVectorWithOffset.h @@ -50,21 +50,14 @@ class NumericVectorWithOffset : public VectorWithOffset typedef VectorWithOffset base_type; public: - //! Construct an empty NumericVectorWithOffset - inline NumericVectorWithOffset(); - - //! Construct a NumericVectorWithOffset of given length - inline explicit NumericVectorWithOffset(const int hsz); - - //! Construct a NumericVectorWithOffset of elements with offset \c min_index - inline NumericVectorWithOffset(const int min_index, const int max_index); + using base_type::base_type; //! Constructor from an object of this class' base_type inline NumericVectorWithOffset(const VectorWithOffset& t); //! Constructor from an object of this class' base_type inline NumericVectorWithOffset(const NumericVectorWithOffset& t) - : NumericVectorWithOffset(static_cast(t)) + : NumericVectorWithOffset(static_cast(t)) {} //! Swap content/members of 2 objects @@ -75,7 +68,7 @@ class NumericVectorWithOffset : public VectorWithOffset } //! move constructor - /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ + /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ NumericVectorWithOffset(NumericVectorWithOffset&& other) noexcept; //! assignment diff --git a/src/include/stir/NumericVectorWithOffset.inl b/src/include/stir/NumericVectorWithOffset.inl index 5fd744596a..70a29b6e42 100644 --- a/src/include/stir/NumericVectorWithOffset.inl +++ b/src/include/stir/NumericVectorWithOffset.inl @@ -27,21 +27,6 @@ START_NAMESPACE_STIR -template -inline NumericVectorWithOffset::NumericVectorWithOffset() - : base_type() -{} - -template -inline NumericVectorWithOffset::NumericVectorWithOffset(const int hsz) - : base_type(hsz) -{} - -template -inline NumericVectorWithOffset::NumericVectorWithOffset(const int min_index, const int max_index) - : base_type(min_index, max_index) -{} - template NumericVectorWithOffset::NumericVectorWithOffset(const VectorWithOffset& t) : base_type(t) diff --git a/src/include/stir/VectorWithOffset.h b/src/include/stir/VectorWithOffset.h index 2ccddc7a57..3b799b9666 100644 --- a/src/include/stir/VectorWithOffset.h +++ b/src/include/stir/VectorWithOffset.h @@ -4,7 +4,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2007-10-08, Hammersmith Imanet Ltd Copyright (C) 2012-06-01 - 2012, Kris Thielemans - Copyright (C) 2023, University College London + Copyright (C) 2023 - 2024, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license @@ -24,6 +24,7 @@ */ #include "stir/shared_ptr.h" +#include "stir/deprecated.h" #include "boost/iterator/iterator_adaptor.hpp" #include "boost/iterator/reverse_iterator.hpp" @@ -138,27 +139,46 @@ class VectorWithOffset //! Construct a VectorWithOffset with offset \c min_index (initialised with \c T()) inline VectorWithOffset(const int min_index, const int max_index); +#if STIR_VERSION < 070000 //! Construct a VectorWithOffset of given length pointing to existing data - inline - VectorWithOffset(const int hsz, T * const data_ptr, T * const end_of_data_ptr); + /*! + \warning This refers to the original memory range, so any modifications to this object will modify + the original data as well. - //! Construct a VectorWithOffset of given length pointing to existing data - inline - VectorWithOffset(const int hsz, T * const data_ptr) - : VectorWithOffset(hsz, data_ptr, data_ptr + hsz) - {} + \deprecated + */ + STIR_DEPRECATED VectorWithOffset(const int hsz, T* const data_ptr, T* const end_of_data_ptr); + + //! Construct a VectorWithOffset with offset \c min_index pointing to existing data + /*! + \warning This refers to the original memory range, so any modifications to this object will modify + the original data as well. + + \deprecated + */ + STIR_DEPRECATED inline VectorWithOffset(const int min_index, const int max_index, T* const data_ptr, T* const end_of_data_ptr); +#endif + + //! Construct a VectorWithOffset of given length from a bare pointer (copying data) + VectorWithOffset(const int hsz, const T* const data_ptr); - //! Construct a VectorWithOffset with offset \c min_index point to existing data - inline - VectorWithOffset(const int min_index, const int max_index, - T * const data_ptr, - T * const end_of_data_ptr); - - //! Construct a VectorWithOffset with offset \c min_index point to existing data - inline - VectorWithOffset(const int min_index, const int max_index, - T * const data_ptr) - : VectorWithOffset(min_index, max_index, data_ptr, data_ptr + (max_index - min_index + 1)) + //! Construct a VectorWithOffset with offset \c min_index from a bare pointer (copying data) + inline VectorWithOffset(const int min_index, const int max_index, const T* const data_ptr); + + //! Construct a VectorWithOffset sharing existing data + /*! + \warning This refers to the original memory range, so any modifications to this object will modify + the original data as well. + */ + inline VectorWithOffset(const int min_index, const int max_index, shared_ptr data_sptr); + + //! Construct a VectorWithOffset sharing existing data + /*! + \warning This refers to the original memory range, so any modifications to this object will modify + the original data as well. + */ + inline VectorWithOffset(const int sz, shared_ptr data_sptr) + : VectorWithOffset(0, sz - 1, data_sptr) {} //! copy constructor @@ -398,8 +418,7 @@ class VectorWithOffset protected: //! pointer to (*this)[0] (taking get_min_index() into account that is). - T* num; // TODOKT make private - shared_ptr allocated_memory_sptr; + T* num; // TODO make private //! Called internally to see if all variables are consistent inline void check_state() const; @@ -418,6 +437,9 @@ class VectorWithOffset T* begin_allocated_memory; T* end_allocated_memory; + //! shared_ptr to memory + shared_ptr allocated_memory_sptr; + //! Default member settings for all constructors inline void init(); diff --git a/src/include/stir/VectorWithOffset.inl b/src/include/stir/VectorWithOffset.inl index e71187f866..7d64e33462 100644 --- a/src/include/stir/VectorWithOffset.inl +++ b/src/include/stir/VectorWithOffset.inl @@ -4,7 +4,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000 - 2010-07-01, Hammersmith Imanet Ltd Copyright (C) 2012-06-01 - 2012, Kris Thielemans - Copyright (C) 2023, University College London + Copyright (C) 2023 - 2024, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license @@ -271,18 +271,14 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index) this->check_state(); } -template -VectorWithOffset::VectorWithOffset(const int sz, T * const data_ptr, T * const end_of_data_ptr) - : VectorWithOffset(0, sz - 1, data_ptr, end_of_data_ptr) -{} - +#if STIR_VERSION < 070000 template VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, T * const data_ptr, T * const end_of_data_ptr) : length(static_cast(max_index - min_index) + 1), start(min_index), pointer_access(false), - allocated_memory_sptr(nullptr) + allocated_memory_sptr(nullptr) // we don't own the data { this->begin_allocated_memory = data_ptr; this->end_allocated_memory = end_of_data_ptr; @@ -291,8 +287,34 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, } template -VectorWithOffset:: -VectorWithOffset(VectorWithOffset&& other) noexcept +VectorWithOffset::VectorWithOffset(const int sz, T* const data_ptr, T* const end_of_data_ptr) + : VectorWithOffset(0, sz - 1, data_ptr, end_of_data_ptr) +{} +#endif // STIR_VERSION < 070000 + +template +VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, const T* const data_ptr) +{ + // first set empty, such that resize() will work ok + this->init(); + // note: need a const_cast, but it's safe because we will copy the data + this->init(min_index, max_index, const_cast(data_ptr), /* copy_data = */ true); +} + +template +VectorWithOffset::VectorWithOffset(const int sz, const T* const data_ptr) + : VectorWithOffset(0, sz - 1, data_ptr) +{} + +template +VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, shared_ptr data_sptr) +{ + this->allocated_memory_sptr = data_sptr; + this->init(min_index, max_index, data_sptr.get(), /* copy_data = */ false); +} + +template +VectorWithOffset::VectorWithOffset(VectorWithOffset&& other) noexcept : VectorWithOffset() { swap(*this, other); diff --git a/src/test/test_Array.cxx b/src/test/test_Array.cxx index 6d00641590..3934bd3e02 100644 --- a/src/test/test_Array.cxx +++ b/src/test/test_Array.cxx @@ -2,7 +2,7 @@ Copyright (C) 2000 PARAPET partners Copyright (C) 2000-2011, Hammersmith Imanet Ltd Copyright (C) 2013 Kris Thielemans - Copyright (C) 2013, 2020, 2023 University College London + Copyright (C) 2013, 2020, 2023, 2024 University College London This file is part of STIR. @@ -279,28 +279,11 @@ class ArrayTests : public RunTests void run_tests() override; }; -template -class Deleter -{ -public: - Deleter(std::vector& v) : - v(v) - {} - void operator()(T* vptr) - { - info("in deleter"); - if (vptr != v.data()) - error("deleter error"); - //~v; - } -private: - std::vector& v; -}; - +// helper function to create a shared_ptr that doesn't delete the data (as it's still owned by the vector) template shared_ptr vec_to_shared(std::vector& v) { - shared_ptr sptr(v.data(), Deleter(v)); + shared_ptr sptr(v.data(), [](auto) {}); return sptr; } @@ -378,8 +361,7 @@ ArrayTests::run_tests() { std::vector mem(test.get_index_range().size_all()); std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); - //Array<1,float> preallocated(test.get_index_range(), &mem[0], false); - Array<1,float> preallocated(test.get_index_range(), vec_to_shared(mem), false); + Array<1, float> preallocated(test.get_index_range(), vec_to_shared(mem)); //shared_ptr mem_sptr(new float [test.get_index_range().size_all()]); //auto mem = mem_sptr.get(); //std::copy(test.begin_all_const(), test.end_all_const(), mem); @@ -417,11 +399,7 @@ ArrayTests::run_tests() { std::vector mem(test.get_index_range().size_all()); std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); - //Array<1,float> test_from_mem(test.get_index_range(), &mem[0], true); - Array<1,float> test_from_mem(test.get_index_range(), vec_to_shared(mem), true); - //shared_ptr mem(test.get_index_range().size_all()); - //std::copy(test.begin_all_const(), test.end_all_const(), mem.get()); - //Array<1,float> preallocated(test.get_index_range(), mem.get(), false); + Array<1, float> test_from_mem(test.get_index_range(), reinterpret_cast(mem.data())); check(test_from_mem.owns_memory_for_data(), "test preallocated with copy: should own memory"); check_if_equal(test, test_from_mem, "test construct from mem: equality"); std::copy(test.begin_all_const(), test.end_all_const(), test_from_mem.begin_all()); @@ -559,7 +537,7 @@ ArrayTests::run_tests() "check row-major order in 2D"); } //Array<2,float> preallocated(t2.get_index_range(), &mem[0], false); - Array<2,float> preallocated(t2.get_index_range(), vec_to_shared(mem), false); + Array<2, float> preallocated(t2.get_index_range(), vec_to_shared(mem)); //check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory"); check_if_equal(t2, preallocated, "test preallocated: equality"); std::copy(t2.begin_all_const(), t2.end_all_const(), preallocated.begin_all()); @@ -1049,7 +1027,7 @@ ArrayTests::run_tests() std::cerr << "vector creation " << t.value()*1000 << "ms\n"; t.start(); //Array<4,int> a1(range, v.data(), false); - Array<4,int> a1(range, vec_to_shared(v), false); + Array<4, int> a1(range, vec_to_shared(v)); t.stop(); //check(!a1.owns_memory_for_data(), "test preallocated without copy: should not own memory"); create_duration = t.value(); From 0ae05b100ac1cf35a313f2de2db2fe604a41d3a6 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Thu, 15 Feb 2024 23:20:45 +0000 Subject: [PATCH 17/25] run clang-format --- src/include/stir/Array.h | 48 ++++++++----- src/include/stir/Array.inl | 57 +++++++-------- src/include/stir/IndexRange.inl | 12 ++-- src/include/stir/NumericVectorWithOffset.inl | 8 +-- src/include/stir/VectorWithOffset.h | 2 +- src/include/stir/VectorWithOffset.inl | 13 ++-- src/include/stir/shared_ptr.h | 18 ++--- src/test/test_Array.cxx | 74 ++++++++++---------- 8 files changed, 124 insertions(+), 108 deletions(-) diff --git a/src/include/stir/Array.h b/src/include/stir/Array.h index 51971c47fe..83ca9e9f26 100644 --- a/src/include/stir/Array.h +++ b/src/include/stir/Array.h @@ -175,11 +175,11 @@ class Array : public NumericVectorWithOffset, e } //! move constructor - /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ + /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ Array(Array&& other) noexcept; //! assignment operator - /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ + /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ Array& operator=(Array other); /*! @name functions returning full_iterators*/ @@ -307,7 +307,7 @@ class Array : public NumericVectorWithOffset, e inline void release_const_full_data_ptr() const; //@} - private: +private: //! boolean to test if get_full_data_ptr is called // This variable is declared mutable such that get_const_full_data_ptr() can change it. mutable bool _full_pointer_access; @@ -322,11 +322,10 @@ class Array : public NumericVectorWithOffset, e The C-array \data_ptr will be accessed with the last dimension running fastest ("row-major" order). */ - inline void - init(const IndexRange& range, elemT * const data_ptr, bool copy_data); + inline void init(const IndexRange& range, elemT* const data_ptr, bool copy_data); // Make sure that we can access init() recursively - template friend class Array; - + template + friend class Array; }; /************************************************** @@ -408,7 +407,7 @@ class Array<1, elemT> : public NumericVectorWithOffset inline Array(const self& t); //! move constructor - /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ + /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ Array(Array&& other) noexcept; //! virtual destructor @@ -461,18 +460,33 @@ class Array<1, elemT> : public NumericVectorWithOffset //! \name access to the data via a pointer //@{ //! return if the array is contiguous in memory (always \c true) - bool is_contiguous() const { return true; } + bool is_contiguous() const + { + return true; + } //! member function for access to the data via a elemT* - inline elemT* get_full_data_ptr() { return this->get_data_ptr(); } + inline elemT* get_full_data_ptr() + { + return this->get_data_ptr(); + } //! member function for access to the data via a const elemT* - inline const elemT* get_const_full_data_ptr() const { return this->get_const_data_ptr(); } + inline const elemT* get_const_full_data_ptr() const + { + return this->get_const_data_ptr(); + } //! signal end of access to elemT* - inline void release_full_data_ptr() { this->release_data_ptr(); } + inline void release_full_data_ptr() + { + this->release_data_ptr(); + } //! signal end of access to const elemT* - inline void release_const_full_data_ptr() const { this->release_const_data_ptr(); } + inline void release_const_full_data_ptr() const + { + this->release_const_data_ptr(); + } //@} //! return sum of all elements @@ -553,16 +567,16 @@ class Array<1, elemT> : public NumericVectorWithOffset inline const elemT& at(const BasicCoordinate<1, int>& c) const; //@} - private: +private: // Make sure we can call init() recursively. - template friend class Array; + template + friend class Array; //! change vector with new index range and point to \c data_ptr /*! \arg data_ptr should start to a contiguous block of correct size */ - inline void init(const IndexRange<1>& range, elemT * const data_ptr, bool copy_data); - + inline void init(const IndexRange<1>& range, elemT* const data_ptr, bool copy_data); }; END_NAMESPACE_STIR diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index 8342dfea49..aa3c11dce9 100644 --- a/src/include/stir/Array.inl +++ b/src/include/stir/Array.inl @@ -38,12 +38,12 @@ bool Array::is_contiguous() const { auto mem = &(*this->begin_all()); - for (auto i=this->get_min_index(); iget_max_index(); ++i) + for (auto i = this->get_min_index(); i < this->get_max_index(); ++i) { if (!(*this)[i].is_contiguous()) return false; mem += (*this)[i].size_all(); - if (mem != &(*(*this)[i+1].begin_all())) + if (mem != &(*(*this)[i + 1].begin_all())) return false; } return true; @@ -62,15 +62,13 @@ Array::resize(const IndexRange& range) template void -Array::init(const IndexRange& range, elemT * const data_ptr, bool copy_data) +Array::init(const IndexRange& range, elemT* const data_ptr, bool copy_data) { base_type::resize(range.get_min_index(), range.get_max_index()); auto iter = this->begin(); auto range_iter = range.begin(); auto ptr = data_ptr; - for (; - iter != this->end(); - ++iter, ++range_iter) + for (; iter != this->end(); ++iter, ++range_iter) { (*iter).init(*range_iter, ptr, copy_data); ptr += range_iter->size_all(); @@ -78,7 +76,7 @@ Array::init(const IndexRange& range, elem } template -void +void Array::grow(const IndexRange& range) { resize(range); @@ -86,29 +84,34 @@ Array::grow(const IndexRange& range) template Array::Array() - : base_type(), _allocated_full_data_ptr(nullptr) + : base_type(), + _allocated_full_data_ptr(nullptr) {} template Array::Array(const IndexRange& range) - : base_type(), _allocated_full_data_ptr(new elemT[range.size_all()]) -{ - // info("Array constructor range " + std::to_string(reinterpret_cast(this->_allocated_full_data_ptr)) + " of size " + std::to_string(range.size_all())); - // set elements to zero - std::for_each(this->_allocated_full_data_ptr.get(), this->_allocated_full_data_ptr.get() + range.size_all(), [](elemT& e) { assign(e, 0); }); + : base_type(), + _allocated_full_data_ptr(new elemT[range.size_all()]) +{ + // info("Array constructor range " + std::to_string(reinterpret_cast(this->_allocated_full_data_ptr)) + " of size " + // + std::to_string(range.size_all())); set elements to zero + std::for_each(this->_allocated_full_data_ptr.get(), this->_allocated_full_data_ptr.get() + range.size_all(), [](elemT& e) { + assign(e, 0); + }); this->init(range, this->_allocated_full_data_ptr.get(), false); } template Array::Array(const IndexRange& range, shared_ptr data_sptr) { - this->_allocated_full_data_ptr = data_sptr; + this->_allocated_full_data_ptr = data_sptr; this->init(range, this->_allocated_full_data_ptr.get(), false); } template Array::Array(const self& t) -: base_type(t), _allocated_full_data_ptr(nullptr) + : base_type(t), + _allocated_full_data_ptr(nullptr) { // info("constructor " + std::to_string(num_dimensions) + "copy of size " + std::to_string(this->size_all())); } @@ -117,7 +120,8 @@ Array::Array(const self& t) // swig cannot parse this ATM, but we don't need it anyway in the wrappers template Array::Array(const base_type& t) -: base_type(t), _allocated_full_data_ptr(nullptr) + : base_type(t), + _allocated_full_data_ptr(nullptr) { // info("constructor basetype " + std::to_string(num_dimensions) + " of size " + std::to_string(this->size_all())); } @@ -128,14 +132,14 @@ Array::~Array() { if (this->_allocated_full_data_ptr) { - // info("Array destructor full_data_ptr " + std::to_string(reinterpret_cast(this->_allocated_full_data_ptr)) + " of size " + std::to_string(this->size_all())); - // delete [] this->_allocated_full_data_ptr; + // info("Array destructor full_data_ptr " + std::to_string(reinterpret_cast(this->_allocated_full_data_ptr)) + + // " of size " + std::to_string(this->size_all())); delete [] this->_allocated_full_data_ptr; } } template Array::Array(Array&& other) noexcept - : Array() + : Array() { swap(*this, other); // info("move constructor " + std::to_string(num_dimensions) + "copy of size " + std::to_string(this->size_all())); @@ -143,8 +147,7 @@ Array::Array(Array&& other) noexce template Array& -Array:: -operator=(Array other) +Array::operator=(Array other) { swap(*this, other); // info("Array= " + std::to_string(num_dimensions) + "copy of size " + std::to_string(this->size_all())); @@ -273,7 +276,7 @@ Array::get_full_data_ptr() \see get_full_data_ptr() */ template -const elemT * +const elemT* Array::get_const_full_data_ptr() const { this->_full_pointer_access = true; @@ -538,10 +541,9 @@ Array::sapyb(const T& a, const Array& y, const T& b) **********************************************/ template void -Array<1, elemT>::init(const IndexRange<1>& range, elemT * const data_ptr, bool copy_data) +Array<1, elemT>::init(const IndexRange<1>& range, elemT* const data_ptr, bool copy_data) { - base_type:: - init(range.get_min_index(), range.get_max_index(), data_ptr, copy_data); + base_type::init(range.get_min_index(), range.get_max_index(), data_ptr, copy_data); } template @@ -625,7 +627,7 @@ Array<1, elemT>::Array(const base_type& il) template Array<1, elemT>::Array(const Array<1, elemT>& other) - : base_type(other) + : base_type(other) {} template @@ -634,7 +636,7 @@ Array<1, elemT>::~Array() template Array<1, elemT>::Array(Array<1, elemT>&& other) noexcept - : Array() + : Array() { swap(*this, other); } @@ -648,7 +650,6 @@ Array<1, elemT>::operator=(const Array<1, elemT>& other) return *this; } - template typename Array<1, elemT>::full_iterator Array<1, elemT>::begin_all() diff --git a/src/include/stir/IndexRange.inl b/src/include/stir/IndexRange.inl index 604f62eff0..8e02a65a26 100644 --- a/src/include/stir/IndexRange.inl +++ b/src/include/stir/IndexRange.inl @@ -71,11 +71,11 @@ std::size_t IndexRange::size_all() const { this->check_state(); - if (this->is_regular_range == regular_true && this->get_length()>0) + if (this->is_regular_range == regular_true && this->get_length() > 0) return this->get_length() * this->begin()->size_all(); - //else - size_t acc=0; - for(int i=this->get_min_index(); i<=this->get_max_index(); i++) + // else + size_t acc = 0; + for (int i = this->get_min_index(); i <= this->get_max_index(); i++) acc += this->num[i].size_all(); return acc; } @@ -166,7 +166,9 @@ IndexRange<1>::get_length() const std::size_t IndexRange<1>::size_all() const -{ return std::size_t(this->get_length()); } +{ + return std::size_t(this->get_length()); +} bool IndexRange<1>::operator==(const IndexRange<1>& range2) const diff --git a/src/include/stir/NumericVectorWithOffset.inl b/src/include/stir/NumericVectorWithOffset.inl index 70a29b6e42..dba3a7a970 100644 --- a/src/include/stir/NumericVectorWithOffset.inl +++ b/src/include/stir/NumericVectorWithOffset.inl @@ -33,9 +33,8 @@ NumericVectorWithOffset::NumericVectorWithOffset(const VectorWithOffs {} template -NumericVectorWithOffset:: -NumericVectorWithOffset(NumericVectorWithOffset&& other) noexcept - : NumericVectorWithOffset() +NumericVectorWithOffset::NumericVectorWithOffset(NumericVectorWithOffset&& other) noexcept + : NumericVectorWithOffset() { swap(*this, other); } @@ -43,8 +42,7 @@ NumericVectorWithOffset(NumericVectorWithOffset&& other) noexcept // assignment template NumericVectorWithOffset& -NumericVectorWithOffset:: -operator=(const NumericVectorWithOffset& other) +NumericVectorWithOffset::operator=(const NumericVectorWithOffset& other) { base_type::operator=(other); return *this; diff --git a/src/include/stir/VectorWithOffset.h b/src/include/stir/VectorWithOffset.h index 3b799b9666..78043ea10b 100644 --- a/src/include/stir/VectorWithOffset.h +++ b/src/include/stir/VectorWithOffset.h @@ -203,7 +203,7 @@ class VectorWithOffset } //! move constructor - /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ + /*! implementation uses the copy-and-swap idiom, see e.g. https://stackoverflow.com/a/3279550 */ VectorWithOffset(VectorWithOffset&& other) noexcept; //! Free all memory and make object as if default-constructed diff --git a/src/include/stir/VectorWithOffset.inl b/src/include/stir/VectorWithOffset.inl index 7d64e33462..ea003af390 100644 --- a/src/include/stir/VectorWithOffset.inl +++ b/src/include/stir/VectorWithOffset.inl @@ -34,8 +34,8 @@ template void VectorWithOffset::init() { - length = 0; // i.e. an empty row of zero length, - start = 0; // no offsets + length = 0; // i.e. an empty row of zero length, + start = 0; // no offsets num = nullptr; // and no data. begin_allocated_memory = nullptr; end_allocated_memory = nullptr; @@ -44,7 +44,7 @@ VectorWithOffset::init() template void -VectorWithOffset::init(const int min_index, const int max_index, T * const data_ptr, bool copy_data) +VectorWithOffset::init(const int min_index, const int max_index, T* const data_ptr, bool copy_data) { this->pointer_access = false; if (copy_data) @@ -99,7 +99,7 @@ VectorWithOffset::_destruct_and_deallocate() // we'll have to be careful to delete only initialised elements // and just de-allocate the rest - this->allocated_memory_sptr = nullptr; + this->allocated_memory_sptr = nullptr; } template @@ -273,8 +273,7 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index) #if STIR_VERSION < 070000 template -VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, - T * const data_ptr, T * const end_of_data_ptr) +VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, T* const data_ptr, T* const end_of_data_ptr) : length(static_cast(max_index - min_index) + 1), start(min_index), pointer_access(false), @@ -315,7 +314,7 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, template VectorWithOffset::VectorWithOffset(VectorWithOffset&& other) noexcept - : VectorWithOffset() + : VectorWithOffset() { swap(*this, other); } diff --git a/src/include/stir/shared_ptr.h b/src/include/stir/shared_ptr.h index dd55747606..8779f2ddae 100644 --- a/src/include/stir/shared_ptr.h +++ b/src/include/stir/shared_ptr.h @@ -35,19 +35,21 @@ using boost::static_pointer_cast; # define MAKE_SHARED boost::make_shared } // namespace stir #else -#if __cplusplus < 201703L -#include -namespace stir { - using std::experimental::shared_ptr; +# if __cplusplus < 201703L +# include +namespace stir +{ +using std::experimental::shared_ptr; } -#else -# include +# else +# include namespace stir { using std::shared_ptr; } -#endif -namespace stir { +# endif +namespace stir +{ using std::dynamic_pointer_cast; using std::static_pointer_cast; //! work-around for using std::make_shared on old compilers diff --git a/src/test/test_Array.cxx b/src/test/test_Array.cxx index 3934bd3e02..8371e4825c 100644 --- a/src/test/test_Array.cxx +++ b/src/test/test_Array.cxx @@ -281,7 +281,8 @@ class ArrayTests : public RunTests // helper function to create a shared_ptr that doesn't delete the data (as it's still owned by the vector) template -shared_ptr vec_to_shared(std::vector& v) +shared_ptr +vec_to_shared(std::vector& v) { shared_ptr sptr(v.data(), [](auto) {}); return sptr; @@ -362,10 +363,10 @@ ArrayTests::run_tests() std::vector mem(test.get_index_range().size_all()); std::copy(test.begin_all_const(), test.end_all_const(), mem.begin()); Array<1, float> preallocated(test.get_index_range(), vec_to_shared(mem)); - //shared_ptr mem_sptr(new float [test.get_index_range().size_all()]); - //auto mem = mem_sptr.get(); - //std::copy(test.begin_all_const(), test.end_all_const(), mem); - //Array<1,float> preallocated(test.get_index_range(), mem_sptr, false); + // shared_ptr mem_sptr(new float [test.get_index_range().size_all()]); + // auto mem = mem_sptr.get(); + // std::copy(test.begin_all_const(), test.end_all_const(), mem); + // Array<1,float> preallocated(test.get_index_range(), mem_sptr, false); check_if_equal(test, preallocated, "test preallocated: equality"); std::copy(test.begin_all_const(), test.end_all_const(), preallocated.begin_all()); check_if_equal(test, preallocated, "test preallocated: copy with full_iterator"); @@ -376,22 +377,22 @@ ArrayTests::run_tests() check(preallocated.get_const_full_data_ptr() == &mem[0], "test Array1D preallocated const pointer access"); preallocated.release_const_full_data_ptr(); // test memory is shared between the Array and mem - mem[0]=*test.begin() + 345; + mem[0] = *test.begin() + 345; check_if_equal(*preallocated.begin(), mem[0], "test preallocated: direct buffer mod"); - *(preallocated.begin()+1) += 4; - check_if_equal(*(preallocated.begin()+1), mem[1], "test preallocated: indirect buffer mod"); + *(preallocated.begin() + 1) += 4; + check_if_equal(*(preallocated.begin() + 1), mem[1], "test preallocated: indirect buffer mod"); // test resize { const auto min = preallocated.get_min_index(); const auto max = preallocated.get_max_index(); // resizing to smaller range will keep pointing to the same memory - preallocated.resize(min+1, max-1); + preallocated.resize(min + 1, max - 1); std::fill(mem.begin(), mem.end(), 12345.F); - check_if_equal(preallocated[min+1], 12345.F, "test preallocated: resize smaller uses same memory"); + check_if_equal(preallocated[min + 1], 12345.F, "test preallocated: resize smaller uses same memory"); // resizing to non-overlapping range will reallocate - preallocated.resize(min-1, max-1); + preallocated.resize(min - 1, max - 1); std::fill(mem.begin(), mem.end(), 123456.F); - check_if_equal(preallocated[min+1], 12345.F, "test preallocated: grow uses different memory"); + check_if_equal(preallocated[min + 1], 12345.F, "test preallocated: grow uses different memory"); } } @@ -405,10 +406,10 @@ ArrayTests::run_tests() std::copy(test.begin_all_const(), test.end_all_const(), test_from_mem.begin_all()); check_if_equal(test, test_from_mem, "test construct from mem: copy with full_iterator"); // test memory is not shared between the Array and mem - mem[0]=*test.begin() + 345; + mem[0] = *test.begin() + 345; check_if_equal(*test_from_mem.begin(), *test.begin(), "test construct from mem: direct buffer mod"); - *(test_from_mem.begin()+1) += 4; - check_if_equal(*(test_from_mem.begin()+1), mem[1]+4, "test construct from mem: indirect buffer mod"); + *(test_from_mem.begin() + 1) += 4; + check_if_equal(*(test_from_mem.begin() + 1), mem[1] + 4, "test construct from mem: indirect buffer mod"); } } #if 1 @@ -517,8 +518,8 @@ ArrayTests::run_tests() { // copy constructor; Array<2, float> t21(t2); - check_if_equal( t21[3][2] , 6.F, "test Array2D copy consructor element"); - check_if_equal(t21 , t2, "test Array2D copy constructor all elements" ); + check_if_equal(t21[3][2], 6.F, "test Array2D copy consructor element"); + check_if_equal(t21, t2, "test Array2D copy constructor all elements"); // 'assignment constructor' (this simply calls copy constructor) Array<2, float> t22 = t2; check_if_equal(t22, t2, "test Array2D copy constructor"); @@ -532,13 +533,12 @@ ArrayTests::run_tests() // test iterator access is row-major auto first_min_idx = t2.get_min_index(); check_if_equal(t2[3][2], - mem[(3 - first_min_idx) * t2[first_min_idx].size_all() + - 2 - t2[first_min_idx].get_min_index()], + mem[(3 - first_min_idx) * t2[first_min_idx].size_all() + 2 - t2[first_min_idx].get_min_index()], "check row-major order in 2D"); } - //Array<2,float> preallocated(t2.get_index_range(), &mem[0], false); + // Array<2,float> preallocated(t2.get_index_range(), &mem[0], false); Array<2, float> preallocated(t2.get_index_range(), vec_to_shared(mem)); - //check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory"); + // check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory"); check_if_equal(t2, preallocated, "test preallocated: equality"); std::copy(t2.begin_all_const(), t2.end_all_const(), preallocated.begin_all()); check_if_equal(t2, preallocated, "test preallocated: copy with full_iterator"); @@ -549,22 +549,22 @@ ArrayTests::run_tests() check(preallocated.get_const_full_data_ptr() == &mem[0], "test Array2D preallocated const pointer access"); preallocated.release_const_full_data_ptr(); // test memory is shared between the Array and mem - mem[0]=*t2.begin_all() + 345; + mem[0] = *t2.begin_all() + 345; check_if_equal(*preallocated.begin_all(), mem[0], "test preallocated: direct buffer mod"); *(preallocated.begin_all()) += 4; check_if_equal(*(preallocated.begin_all()), mem[0], "test preallocated: indirect buffer mod"); // test resize { - BasicCoordinate<2,int> min, max; + BasicCoordinate<2, int> min, max; preallocated.get_regular_range(min, max); // resizing to smaller range will keep pointing to the same memory - preallocated.resize(IndexRange<2>(min+1, max-1)); + preallocated.resize(IndexRange<2>(min + 1, max - 1)); std::fill(mem.begin(), mem.end(), 12345.F); - check_if_equal(preallocated[min+1], 12345.F, "test preallocated: resize smaller uses same memory"); + check_if_equal(preallocated[min + 1], 12345.F, "test preallocated: resize smaller uses same memory"); check(!preallocated.is_contiguous(), "test preallocated: no longer contiguous after resize"); - preallocated.resize(IndexRange<2>(min-1, max-1)); + preallocated.resize(IndexRange<2>(min - 1, max - 1)); std::fill(mem.begin(), mem.end(), 123456.F); - check_if_equal(preallocated[min+1], 12345.F, "test preallocated: grow uses different memory"); + check_if_equal(preallocated[min + 1], 12345.F, "test preallocated: grow uses different memory"); } } } @@ -1003,39 +1003,39 @@ ArrayTests::run_tests() std::cerr << "timings\n"; { HighResWallClockTimer t; - IndexRange<4> range(Coordinate4D(20,100,400,600)); + IndexRange<4> range(Coordinate4D(20, 100, 400, 600)); t.start(); double create_duration; { - Array<4,int> a1(range); + Array<4, int> a1(range); t.stop(); - std::cerr << "creation of non-contiguous 4D Array " << t.value()*1000 << "ms\n"; + std::cerr << "creation of non-contiguous 4D Array " << t.value() * 1000 << "ms\n"; create_duration = t.value(); t.start(); } t.stop(); - std::cerr << "deletion " << (t.value() - create_duration)*1000 << " ms\n"; + std::cerr << "deletion " << (t.value() - create_duration) * 1000 << " ms\n"; t.reset(); t.start(); { const auto s = range.size_all(); t.stop(); - std::cerr << "range size_all " << t.value()*1000 << "ms\n"; + std::cerr << "range size_all " << t.value() * 1000 << "ms\n"; t.start(); std::vector v(s, 0); t.stop(); - std::cerr << "vector creation " << t.value()*1000 << "ms\n"; + std::cerr << "vector creation " << t.value() * 1000 << "ms\n"; t.start(); - //Array<4,int> a1(range, v.data(), false); + // Array<4,int> a1(range, v.data(), false); Array<4, int> a1(range, vec_to_shared(v)); t.stop(); - //check(!a1.owns_memory_for_data(), "test preallocated without copy: should not own memory"); + // check(!a1.owns_memory_for_data(), "test preallocated without copy: should not own memory"); create_duration = t.value(); - std::cerr << "contiguous array creation (total) " << t.value()*1000 << "ms\n"; + std::cerr << "contiguous array creation (total) " << t.value() * 1000 << "ms\n"; t.start(); } t.stop(); - std::cerr << "deletion " << (t.value() - create_duration)*1000 << " ms\n"; + std::cerr << "deletion " << (t.value() - create_duration) * 1000 << " ms\n"; } } From 345b0d7f1a2467b904ba01da467fbf226c31adb8 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Fri, 16 Feb 2024 08:36:43 +0000 Subject: [PATCH 18/25] remove experimental/memory work-around for shared_ptr This was an attempt to handle pre-C++-17, but it would need work for make_shared. In any case, it isn't available on VS compilers apparently. --- src/include/stir/shared_ptr.h | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/include/stir/shared_ptr.h b/src/include/stir/shared_ptr.h index 8779f2ddae..a24edfe3cd 100644 --- a/src/include/stir/shared_ptr.h +++ b/src/include/stir/shared_ptr.h @@ -35,21 +35,12 @@ using boost::static_pointer_cast; # define MAKE_SHARED boost::make_shared } // namespace stir #else -# if __cplusplus < 201703L -# include -namespace stir -{ -using std::experimental::shared_ptr; -} -# else -# include + +# include + namespace stir { using std::shared_ptr; -} -# endif -namespace stir -{ using std::dynamic_pointer_cast; using std::static_pointer_cast; //! work-around for using std::make_shared on old compilers From be32661711cb9354d88811d0f022e749fae76723 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Fri, 16 Feb 2024 11:58:41 +0000 Subject: [PATCH 19/25] remove VS 2015 job as no C++-17 for shared_ptr --- .appveyor.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.appveyor.yml b/.appveyor.yml index 326e4515a6..a613b19937 100644 --- a/.appveyor.yml +++ b/.appveyor.yml @@ -14,7 +14,6 @@ version: '{build}' os: - Visual Studio 2022 - Visual Studio 2019 - - Visual Studio 2015 platform: - x64 From 510e4294f875c1cd111ba3531dacedd15a992e52 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Sat, 17 Feb 2024 15:51:42 +0000 Subject: [PATCH 20/25] [CMake] fix FindCERN_ROOT to always check version This was previously only done when finding the CONFIG file --- src/cmake/FindCERN_ROOT.cmake | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cmake/FindCERN_ROOT.cmake b/src/cmake/FindCERN_ROOT.cmake index 92c07159b7..4879779640 100644 --- a/src/cmake/FindCERN_ROOT.cmake +++ b/src/cmake/FindCERN_ROOT.cmake @@ -168,4 +168,6 @@ if (CERN_ROOT_DEBUG) endif() INCLUDE(FindPackageHandleStandardArgs) -FIND_PACKAGE_HANDLE_STANDARD_ARGS(CERN_ROOT "CERN ROOT not found. If you do have it, set ROOT_DIR (preferred), ROOTSYS or add root-config to your path" CERN_ROOT_VERSION CERN_ROOT_LIBRARIES CERN_ROOT_INCLUDE_DIRS) +FIND_PACKAGE_HANDLE_STANDARD_ARGS(CERN_ROOT FAIL_MESSAGE "CERN ROOT not found. If you do have it, set ROOT_DIR (preferred), ROOTSYS or add root-config to your path" + VERSION_VAR CERN_ROOT_VERSION + REQUIRED_VARS CERN_ROOT_LIBRARIES CERN_ROOT_INCLUDE_DIRS) From 715ede810f8734841a9fd754777b61593ab80dbc Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Sat, 17 Feb 2024 15:52:59 +0000 Subject: [PATCH 21/25] [CMake] require ROOT 6.28.0 (needed for C++-17) --- CMakeLists.txt | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7343db929e..47275ae2d1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -118,18 +118,12 @@ if(NOT DISABLE_LLN_MATRIX) endif() if(NOT DISABLE_CERN_ROOT) - find_package(CERN_ROOT) + find_package(CERN_ROOT 6.28.00) # required for C++17 if (CERN_ROOT_FOUND) message(STATUS "ROOT Version: ${CERN_ROOT_VERSION}") if (${CERN_ROOT_VERSION} VERSION_GREATER 6.29.99) message(STATUS "ROOT Version is >= 6.30. Setting the minimum CXX version to 20.") UseCXX(20) - elseif (${CERN_ROOT_VERSION} VERSION_GREATER 6.27.99) - message(STATUS "ROOT Version is >= 6.28. Setting the minimum CXX version to 17.") - UseCXX(17) - elseif (${CERN_ROOT_VERSION} VERSION_GREATER 6.23.99) - message(STATUS "ROOT Version is >= 6.24. Setting the minimum CXX version to 14.") - UseCXX(14) endif() endif() endif() From 4bf2d1345e294a31f2f37ca3325a98d3d8e1ed21 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Fri, 17 May 2024 13:09:12 +0100 Subject: [PATCH 22/25] optimise reading/writing for contiguous arrays --- src/include/stir/IO/read_data.h | 1 + src/include/stir/IO/read_data.inl | 5 +++ src/include/stir/IO/read_data_1d.h | 8 ++--- src/include/stir/IO/read_data_1d.inl | 29 ++++++++-------- src/include/stir/IO/write_data_1d.h | 9 ++--- src/include/stir/IO/write_data_1d.inl | 49 ++++++++++++++------------- 6 files changed, 55 insertions(+), 46 deletions(-) diff --git a/src/include/stir/IO/read_data.h b/src/include/stir/IO/read_data.h index a898eeb5e9..1baefc8185 100644 --- a/src/include/stir/IO/read_data.h +++ b/src/include/stir/IO/read_data.h @@ -2,6 +2,7 @@ #define __stir_IO_read_data_H__ /* Copyright (C) 2004- 2007, Hammersmith Imanet Ltd + Copyright (C) 2024, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 diff --git a/src/include/stir/IO/read_data.inl b/src/include/stir/IO/read_data.inl index 7351576c88..e8c6b30818 100644 --- a/src/include/stir/IO/read_data.inl +++ b/src/include/stir/IO/read_data.inl @@ -1,5 +1,6 @@ /* Copyright (C) 2004- 2007, Hammersmith Imanet Ltd + Copyright (C) 2024, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 @@ -34,6 +35,10 @@ template inline Succeeded read_data_help(is_not_1d, IStreamT& s, Array& data, const ByteOrder byte_order) { + if (data.is_contiguous()) + return read_data_1d(s, data, byte_order); + + // otherwise, recurse for (typename Array::iterator iter = data.begin(); iter != data.end(); ++iter) { if (read_data(s, *iter, byte_order) == Succeeded::no) diff --git a/src/include/stir/IO/read_data_1d.h b/src/include/stir/IO/read_data_1d.h index 0bd70136e6..1784df7eaf 100644 --- a/src/include/stir/IO/read_data_1d.h +++ b/src/include/stir/IO/read_data_1d.h @@ -34,15 +34,15 @@ namespace detail This function might propagate any exceptions by std::istream::read. */ -template -inline Succeeded read_data_1d(std::istream& s, Array<1, elemT>& data, const ByteOrder byte_order); +template +inline Succeeded read_data_1d(std::istream& s, Array& data, const ByteOrder byte_order); /* \ingroup Array_IO_detail \brief This is the (internal) function that does the actual reading from a FILE*. \internal */ -template -inline Succeeded read_data_1d(FILE*&, Array<1, elemT>& data, const ByteOrder byte_order); +template +inline Succeeded read_data_1d(FILE*&, Array& data, const ByteOrder byte_order); } // end namespace detail END_NAMESPACE_STIR diff --git a/src/include/stir/IO/read_data_1d.inl b/src/include/stir/IO/read_data_1d.inl index d0c8a2d713..0c841cfda2 100644 --- a/src/include/stir/IO/read_data_1d.inl +++ b/src/include/stir/IO/read_data_1d.inl @@ -8,6 +8,7 @@ */ /* Copyright (C) 2004- 2009, Hammersmith Imanet Ltd + Copyright (C) 2024, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 @@ -27,9 +28,9 @@ namespace detail /***************** version for istream *******************************/ -template +template Succeeded -read_data_1d(std::istream& s, Array<1, elemT>& data, const ByteOrder byte_order) +read_data_1d(std::istream& s, Array& data, const ByteOrder byte_order) { if (!s || (dynamic_cast(&s) != 0 && !dynamic_cast(&s)->is_open()) || (dynamic_cast(&s) != 0 && !dynamic_cast(&s)->is_open())) @@ -41,9 +42,9 @@ read_data_1d(std::istream& s, Array<1, elemT>& data, const ByteOrder byte_order) // note: find num_to_read (using size()) outside of s.read() function call // otherwise Array::check_state() in size() might abort if // get_data_ptr() is called before size() (which is compiler dependent) - const std::streamsize num_to_read = static_cast(data.size()) * sizeof(elemT); - s.read(reinterpret_cast(data.get_data_ptr()), num_to_read); - data.release_data_ptr(); + const std::streamsize num_to_read = static_cast(data.size_all()) * sizeof(elemT); + s.read(reinterpret_cast(data.get_full_data_ptr()), num_to_read); + data.release_full_data_ptr(); if (!s) { @@ -53,8 +54,8 @@ read_data_1d(std::istream& s, Array<1, elemT>& data, const ByteOrder byte_order) if (!byte_order.is_native_order()) { - for (int i = data.get_min_index(); i <= data.get_max_index(); ++i) - ByteOrder::swap_order(data[i]); + for (auto iter = data.begin_all(); iter != data.end_all(); ++iter) + ByteOrder::swap_order(*iter); } return Succeeded::yes; @@ -63,9 +64,9 @@ read_data_1d(std::istream& s, Array<1, elemT>& data, const ByteOrder byte_order) /***************** version for FILE *******************************/ // largely a copy of above, but with calls to stdio function -template +template Succeeded -read_data_1d(FILE*& fptr_ref, Array<1, elemT>& data, const ByteOrder byte_order) +read_data_1d(FILE*& fptr_ref, Array& data, const ByteOrder byte_order) { FILE* fptr = fptr_ref; if (fptr == NULL || ferror(fptr)) @@ -77,9 +78,9 @@ read_data_1d(FILE*& fptr_ref, Array<1, elemT>& data, const ByteOrder byte_order) // note: find num_to_read (using size()) outside of s.read() function call // otherwise Array::check_state() in size() might abort if // get_data_ptr() is called before size() (which is compiler dependent) - const std::size_t num_to_read = static_cast(data.size()); - const std::size_t num_read = fread(reinterpret_cast(data.get_data_ptr()), sizeof(elemT), num_to_read, fptr); - data.release_data_ptr(); + const std::size_t num_to_read = static_cast(data.size_all()); + const std::size_t num_read = fread(reinterpret_cast(data.get_full_data_ptr()), sizeof(elemT), num_to_read, fptr); + data.release_full_data_ptr(); if (ferror(fptr) || num_to_read != num_read) { @@ -89,8 +90,8 @@ read_data_1d(FILE*& fptr_ref, Array<1, elemT>& data, const ByteOrder byte_order) if (!byte_order.is_native_order()) { - for (int i = data.get_min_index(); i <= data.get_max_index(); ++i) - ByteOrder::swap_order(data[i]); + for (auto iter = data.begin_all(); iter != data.end_all(); ++iter) + ByteOrder::swap_order(*iter); } return Succeeded::yes; diff --git a/src/include/stir/IO/write_data_1d.h b/src/include/stir/IO/write_data_1d.h index e376210366..e96c8c05b9 100644 --- a/src/include/stir/IO/write_data_1d.h +++ b/src/include/stir/IO/write_data_1d.h @@ -10,6 +10,7 @@ */ /* Copyright (C) 2004- 2009, Hammersmith Imanet Ltd + Copyright (C) 2024, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 @@ -35,9 +36,9 @@ namespace detail This function does not throw any exceptions. Exceptions thrown by std::ostream::write are caught. */ -template +template inline Succeeded -write_data_1d(std::ostream& s, const Array<1, elemT>& data, const ByteOrder byte_order, const bool can_corrupt_data); +write_data_1d(std::ostream& s, const Array& data, const ByteOrder byte_order, const bool can_corrupt_data); /*! \ingroup Array_IO_detail \brief This is an internal function called by \c write_data(). It does the actual writing to \c FILE* using stdio functions. @@ -45,9 +46,9 @@ write_data_1d(std::ostream& s, const Array<1, elemT>& data, const ByteOrder byte This function does not throw any exceptions. */ -template +template inline Succeeded -write_data_1d(FILE*& fptr_ref, const Array<1, elemT>& data, const ByteOrder byte_order, const bool can_corrupt_data); +write_data_1d(FILE*& fptr_ref, const Array& data, const ByteOrder byte_order, const bool can_corrupt_data); } // namespace detail END_NAMESPACE_STIR diff --git a/src/include/stir/IO/write_data_1d.inl b/src/include/stir/IO/write_data_1d.inl index 8a704d9852..b1a0a9c291 100644 --- a/src/include/stir/IO/write_data_1d.inl +++ b/src/include/stir/IO/write_data_1d.inl @@ -8,6 +8,7 @@ */ /* Copyright (C) 2004- 2009, Hammersmith Imanet Ltd + Copyright (C) 2024, University College London This file is part of STIR. SPDX-License-Identifier: Apache-2.0 @@ -27,9 +28,9 @@ namespace detail /***************** version for ostream *******************************/ -template +template inline Succeeded -write_data_1d(std::ostream& s, const Array<1, elemT>& data, const ByteOrder byte_order, const bool can_corrupt_data) +write_data_1d(std::ostream& s, const Array& data, const ByteOrder byte_order, const bool can_corrupt_data) { if (!s || (dynamic_cast(&s) != 0 && !dynamic_cast(&s)->is_open()) || (dynamic_cast(&s) != 0 && !dynamic_cast(&s)->is_open())) @@ -44,7 +45,7 @@ write_data_1d(std::ostream& s, const Array<1, elemT>& data, const ByteOrder byte /* if (!byte_order.is_native_order()) { - Array<1, elemT> a_copy(data); + Array a_copy(data); for(int i=data.get_min_index(); i<=data.get_max_index(); i++) ByteOrder::swap_order(a_copy[i]); return write_data(s, a_copy, ByteOrder::native, true); @@ -52,32 +53,32 @@ write_data_1d(std::ostream& s, const Array<1, elemT>& data, const ByteOrder byte */ if (!byte_order.is_native_order()) { - Array<1, elemT>& data_ref = const_cast&>(data); - for (int i = data.get_min_index(); i <= data.get_max_index(); ++i) - ByteOrder::swap_order(data_ref[i]); + Array& data_ref = const_cast&>(data); + for (auto iter = data_ref.begin_all(); iter != data_ref.end_all(); ++iter) + ByteOrder::swap_order(*iter); } // note: find num_to_write (using size()) outside of s.write() function call // otherwise Array::check_state() in size() might abort if // get_const_data_ptr() is called before size() (which is compiler dependent) - const std::streamsize num_to_write = static_cast(data.size()) * sizeof(elemT); + const std::streamsize num_to_write = static_cast(data.size_all()) * sizeof(elemT); bool writing_ok = true; try { - s.write(reinterpret_cast(data.get_const_data_ptr()), num_to_write); + s.write(reinterpret_cast(data.get_const_full_data_ptr()), num_to_write); } catch (...) { writing_ok = false; } - data.release_const_data_ptr(); + data.release_const_full_data_ptr(); if (!can_corrupt_data && !byte_order.is_native_order()) { - Array<1, elemT>& data_ref = const_cast&>(data); - for (int i = data.get_min_index(); i <= data.get_max_index(); ++i) - ByteOrder::swap_order(data_ref[i]); + Array& data_ref = const_cast&>(data); + for (auto iter = data_ref.begin_all(); iter != data_ref.end_all(); ++iter) + ByteOrder::swap_order(*iter); } if (!writing_ok || !s) @@ -92,9 +93,9 @@ write_data_1d(std::ostream& s, const Array<1, elemT>& data, const ByteOrder byte /***************** version for FILE *******************************/ // largely a copy of above, but with calls to stdio function -template +template inline Succeeded -write_data_1d(FILE*& fptr_ref, const Array<1, elemT>& data, const ByteOrder byte_order, const bool can_corrupt_data) +write_data_1d(FILE*& fptr_ref, const Array& data, const ByteOrder byte_order, const bool can_corrupt_data) { FILE* fptr = fptr_ref; if (fptr == 0 || ferror(fptr)) @@ -109,7 +110,7 @@ write_data_1d(FILE*& fptr_ref, const Array<1, elemT>& data, const ByteOrder byte /* if (!byte_order.is_native_order()) { - Array<1, elemT> a_copy(data); + Array a_copy(data); for(int i=data.get_min_index(); i<=data.get_max_index(); i++) ByteOrder::swap_order(a_copy[i]); return write_data(s, a_copy, ByteOrder::native, true); @@ -117,25 +118,25 @@ write_data_1d(FILE*& fptr_ref, const Array<1, elemT>& data, const ByteOrder byte */ if (!byte_order.is_native_order()) { - Array<1, elemT>& data_ref = const_cast&>(data); - for (int i = data.get_min_index(); i <= data.get_max_index(); ++i) - ByteOrder::swap_order(data_ref[i]); + Array& data_ref = const_cast&>(data); + for (auto iter = data_ref.begin_all(); iter != data_ref.end_all(); ++iter) + ByteOrder::swap_order(*iter); } // note: find num_to_write (using size()) outside of s.write() function call // otherwise Array::check_state() in size() might abort if // get_const_data_ptr() is called before size() (which is compiler dependent) - const std::size_t num_to_write = static_cast(data.size()); + const std::size_t num_to_write = static_cast(data.size_all()); const std::size_t num_written - = fwrite(reinterpret_cast(data.get_const_data_ptr()), sizeof(elemT), num_to_write, fptr); + = fwrite(reinterpret_cast(data.get_const_full_data_ptr()), sizeof(elemT), num_to_write, fptr); - data.release_const_data_ptr(); + data.release_const_full_data_ptr(); if (!can_corrupt_data && !byte_order.is_native_order()) { - Array<1, elemT>& data_ref = const_cast&>(data); - for (int i = data.get_min_index(); i <= data.get_max_index(); ++i) - ByteOrder::swap_order(data_ref[i]); + Array& data_ref = const_cast&>(data); + for (auto iter = data_ref.begin_all(); iter != data_ref.end_all(); ++iter) + ByteOrder::swap_order(*iter); } if (num_written != num_to_write || ferror(fptr)) From 74579afced8acd4eeb1a48fc8a7bac8b88908f68 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Fri, 17 May 2024 20:40:28 +0100 Subject: [PATCH 23/25] [SWIG] avoid warning on stir::swap --- src/swig/stir.i | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/swig/stir.i b/src/swig/stir.i index 437e994da7..a90824e9b5 100644 --- a/src/swig/stir.i +++ b/src/swig/stir.i @@ -576,7 +576,10 @@ %newobject *::get_empty_copy; %newobject *::read_from_file; +// SWIG complains "Warning 503: Can't wrap 'stir::swap' unless renamed to a valid identifier." +// But it's probably dangerous to expose swap anyway, so let's ignore it. %ignore **::swap; +%ignore stir::swap; %ignore *::ask_parameters; %ignore *::create_shared_clone; %ignore *::read_from_stream; From 8752e454fac3214539a694536a2661f0cd14c337 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Fri, 17 May 2024 22:19:21 +0100 Subject: [PATCH 24/25] Avoid image copies in Parallelproj if data is contiguous, we don't need an extra copy. --- .../BackProjectorByBinParallelproj.cxx | 30 +++++++++++++++---- .../ForwardProjectorByBinParallelproj.cxx | 24 +++++++++++---- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/recon_buildblock/Parallelproj_projector/BackProjectorByBinParallelproj.cxx b/src/recon_buildblock/Parallelproj_projector/BackProjectorByBinParallelproj.cxx index 2589c62b4d..474372018a 100644 --- a/src/recon_buildblock/Parallelproj_projector/BackProjectorByBinParallelproj.cxx +++ b/src/recon_buildblock/Parallelproj_projector/BackProjectorByBinParallelproj.cxx @@ -133,8 +133,19 @@ TOF_transpose(std::vector& mem_for_PP_back, void BackProjectorByBinParallelproj::get_output(DiscretisedDensity<3, float>& density) const { + std::vector image_vec; + float* image_ptr; + if (_density_sptr->is_contiguous()) + { + image_ptr = _density_sptr->get_full_data_ptr(); + } + else + { + image_vec.resize(density.size_all()); + std::copy(_density_sptr->begin_all(), _density_sptr->end_all(), image_vec.begin()); + image_ptr = image_vec.data(); + } - std::vector image_vec(density.size_all()); // create an alias for the projection data const ProjDataInMemory& p(*_proj_data_to_backproject_sptr); @@ -149,7 +160,7 @@ BackProjectorByBinParallelproj::get_output(DiscretisedDensity<3, float>& density long long offset = 0; // send image to all visible CUDA devices - float** image_on_cuda_devices = copy_float_array_to_all_devices(image_vec.data(), _helper->num_image_voxel); + float** image_on_cuda_devices = copy_float_array_to_all_devices(image_ptr, _helper->num_image_voxel); // do (chuck-wise) back projection on the CUDA devices for (int chunk_num = 0; chunk_num < _num_gpu_chunks; chunk_num++) @@ -211,7 +222,7 @@ BackProjectorByBinParallelproj::get_output(DiscretisedDensity<3, float>& density sum_float_arrays_on_first_device(image_on_cuda_devices, _helper->num_image_voxel); // copy summed image back to host - get_float_array_from_device(image_on_cuda_devices, _helper->num_image_voxel, 0, image_vec.data()); + get_float_array_from_device(image_on_cuda_devices, _helper->num_image_voxel, 0, image_ptr); // free image array from CUDA devices free_float_array_on_all_devices(image_on_cuda_devices); @@ -226,7 +237,7 @@ BackProjectorByBinParallelproj::get_output(DiscretisedDensity<3, float>& density joseph3d_back_tof_sino(_helper->xend.data(), _helper->xstart.data(), - image_vec.data(), + image_ptr, _helper->origin.data(), _helper->voxsize.data(), mem_for_PP_back.data(), @@ -245,7 +256,7 @@ BackProjectorByBinParallelproj::get_output(DiscretisedDensity<3, float>& density { joseph3d_back(_helper->xstart.data(), _helper->xend.data(), - image_vec.data(), + image_ptr, _helper->origin.data(), _helper->voxsize.data(), p.get_const_data_ptr(), @@ -260,7 +271,14 @@ BackProjectorByBinParallelproj::get_output(DiscretisedDensity<3, float>& density // --------------------------------------------------------------- // // Parallelproj -> STIR image conversion // --------------------------------------------------------------- // - std::copy(image_vec.begin(), image_vec.end(), density.begin_all()); + if (_density_sptr->is_contiguous()) + { + _density_sptr->release_full_data_ptr(); + } + else + { + std::copy(image_vec.begin(), image_vec.end(), density.begin_all()); + } // After the back projection, we enforce a truncation outside of the FOV. // This is because the parallelproj projector seems to have some trouble at the edges and this diff --git a/src/recon_buildblock/Parallelproj_projector/ForwardProjectorByBinParallelproj.cxx b/src/recon_buildblock/Parallelproj_projector/ForwardProjectorByBinParallelproj.cxx index 9c445a9d1f..ac4a3704c1 100644 --- a/src/recon_buildblock/Parallelproj_projector/ForwardProjectorByBinParallelproj.cxx +++ b/src/recon_buildblock/Parallelproj_projector/ForwardProjectorByBinParallelproj.cxx @@ -154,8 +154,18 @@ ForwardProjectorByBinParallelproj::set_input(const DiscretisedDensity<3, float>& truncate_rim(*_density_sptr, static_cast(std::max((image_radius - radius) / _helper->voxsize[2], 0.F))); } - std::vector image_vec(density.size_all()); - std::copy(_density_sptr->begin_all(), _density_sptr->end_all(), image_vec.begin()); + std::vector image_vec; + float* image_ptr; + if (_density_sptr->is_contiguous()) + { + image_ptr = _density_sptr->get_full_data_ptr(); + } + else + { + image_vec.resize(density.size_all()); + std::copy(_density_sptr->begin_all(), _density_sptr->end_all(), image_vec.begin()); + image_ptr = image_vec.data(); + } #if 0 // needed to set output to zero as parallelproj accumulates but is no longer the case @@ -174,7 +184,7 @@ ForwardProjectorByBinParallelproj::set_input(const DiscretisedDensity<3, float>& long long offset = 0; // send image to all visible CUDA devices - float** image_on_cuda_devices = copy_float_array_to_all_devices(image_vec.data(), _helper->num_image_voxel); + float** image_on_cuda_devices = copy_float_array_to_all_devices(image_ptr, _helper->num_image_voxel); // do (chuck-wise) projection on the CUDA devices for (int chunk_num = 0; chunk_num < _num_gpu_chunks; chunk_num++) @@ -246,7 +256,7 @@ ForwardProjectorByBinParallelproj::set_input(const DiscretisedDensity<3, float>& std::vector mem_for_PP(_helper->num_lors * _helper->num_tof_bins); joseph3d_fwd_tof_sino(_helper->xend.data(), _helper->xstart.data(), - image_vec.data(), + image_ptr, _helper->origin.data(), _helper->voxsize.data(), mem_for_PP.data(), @@ -268,7 +278,7 @@ ForwardProjectorByBinParallelproj::set_input(const DiscretisedDensity<3, float>& { joseph3d_fwd(_helper->xstart.data(), _helper->xend.data(), - image_vec.data(), + image_ptr, _helper->origin.data(), _helper->voxsize.data(), _projected_data_sptr->get_data_ptr(), @@ -278,6 +288,10 @@ ForwardProjectorByBinParallelproj::set_input(const DiscretisedDensity<3, float>& #endif info("done", 2); + if (_density_sptr->is_contiguous()) + { + _density_sptr->release_full_data_ptr(); + } _projected_data_sptr->release_data_ptr(); } From ae828264c79c7acf844cda2c540b1cd7b2276a15 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Thu, 23 May 2024 23:18:01 +0100 Subject: [PATCH 25/25] created release_6.2.htm with info on Array PR --- documentation/release_6.2.htm | 125 ++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 documentation/release_6.2.htm diff --git a/documentation/release_6.2.htm b/documentation/release_6.2.htm new file mode 100644 index 0000000000..b734674603 --- /dev/null +++ b/documentation/release_6.2.htm @@ -0,0 +1,125 @@ + + + + Summary of changes in STIR release 6.2 + + + +

Summary of changes in STIR release 6.2

+ +

+ This version is 100% backwards compatible with STIR 6.1. However, C++-17 is now required. +

+ +

Overall summary

+

+ +

+ +

+ Of course, there is also the usual code-cleanup and improvements to the documentation. +

+ +

+ This release contains mainly code written by Nicole Jurjew (UCL) and Kris Thielemans (UCL). +

+ +

Patch release info

+ + +

Summary for end users (also to be read by developers)

+ + +

New functionality

+
    +
+ + +

Changed functionality

+
    +
+ + +

Bug fixes

+
    +
+ +

Build system

+
    + C++-17 is now required. + +
+ +

Known problems

+

See our issue tracker.

+ + +

What's new for developers (aside from what should be obvious +from the above):

+ +

Changed functionality

+
    +
  • + Array classes by default use contiguous memory allocation (as opposed to a sequence of 1D vectors). + This could speed up memory allocation and destruction of arrays with a high number of underlying 1D vectors. It also allows reading/writing + data in one call to the C++ library, as opposed to many small calls. Also added move constructors to the Array, + VectorWithOffset classes. +
    + issue #1236. +
    • +
+ +

Bug fixes

+
    +
  • + PoissonLogLikelihoodWithLinearModelForMeanAndProjData had a (minor?) problem with TOF data + that when computing the gradient, the normalisation object was not set-up with the TOF data, + but non-TOF instead. This did not happen in our normal reconstruction code, and would have thrown an error + if it occured. +
    + Fixed in issue #1427. +
    • +
+ + +

Other code changes

+
    +
  • + Fixes an incompatibility with C++20. +
    • +
+ +

Build system

+
    +
  • + Force C++ version according to CERN ROOT versions: ROOT 6.28.10 needs C++17 and 6.30.2 needs C++20. + Also some fixes when relying on root-config. +
    • +
+ +

Test changes

+ +

C++ tests

+
    +
  • + Objective functions (both projection-data and list-mode) and priors now have a numerical test for accumulate_Hessian_times_input +
    + PR #1418 +
    • +
+ + + +