diff --git a/dimod/include/dimod/iterators.h b/dimod/include/dimod/iterators.h index 74225e444..e9aef2184 100644 --- a/dimod/include/dimod/iterators.h +++ b/dimod/include/dimod/iterators.h @@ -18,317 +18,9 @@ namespace dimod { -template -class Neighborhood; - template class QuadraticModelBase; -template -class NeighborhoodIterator { - public: - using bias_type = Bias; - using index_type = Index; - using neighborhood_type = Neighborhood; - - using value_type = std::pair; - using pointer = value_type*; - using reference = value_type&; - using difference_type = std::ptrdiff_t; - using iterator_category = std::random_access_iterator_tag; - - // empty constructor needed for Cython - NeighborhoodIterator() - : neighborhood_ptr_(nullptr), i_(0), pair_ptr_(nullptr) {} - - // for the copy constructor, we want to copy the pointer to the - // neighborhood, but not to the pair_ptr_, since that's managed by each - // iterator separately - NeighborhoodIterator(const NeighborhoodIterator& other) - : neighborhood_ptr_(other.neighborhood_ptr_), - i_(other.i_), - pair_ptr_(nullptr) { - update_pair(); - } - - // for the move constructor, we want to move everything - NeighborhoodIterator(NeighborhoodIterator&& other) noexcept - : neighborhood_ptr_(nullptr), i_(0), pair_ptr_(nullptr) { - swap(other); - } - - NeighborhoodIterator(neighborhood_type* neighborhood, index_type i) - : neighborhood_ptr_(neighborhood), i_(i), pair_ptr_(nullptr) { - update_pair(); - } - - ~NeighborhoodIterator() { delete pair_ptr_; } - - NeighborhoodIterator& operator=( - NeighborhoodIterator const& other) { - if (this != &other) { - neighborhood_ptr_ = other.neighborhood_ptr_; - i_ = other.i_; - update_pair(); // handles pair_ptr_ - } - return *this; - } - - NeighborhoodIterator& operator=( - NeighborhoodIterator&& other) noexcept { - swap(other); - return *this; - } - - reference operator*() { return *pair_ptr_; } - - pointer operator->() { return pair_ptr_; } - - NeighborhoodIterator& operator++() { - ++i_; - update_pair(); - return *this; - } - NeighborhoodIterator& operator--() { - --i_; - update_pair(); - return *this; - } - NeighborhoodIterator operator++(int) { - NeighborhoodIterator r(*this); - ++i_; - update_pair(); - return r; - } - NeighborhoodIterator operator--(int) { - NeighborhoodIterator r(*this); - --i_; - update_pair(); - return r; - } - - NeighborhoodIterator& operator+=(int n) { - i_ += n; - update_pair(); - return *this; - } - NeighborhoodIterator& operator-=(int n) { - i_ -= n; - update_pair(); - return *this; - } - - NeighborhoodIterator operator+(int n) const { - NeighborhoodIterator r(*this); - return r += n; - } - NeighborhoodIterator operator-(int n) const { - NeighborhoodIterator r(*this); - return r -= n; - } - - difference_type operator-( - NeighborhoodIterator const& r) const { - return i_ - r.i_; - } - - bool operator<(NeighborhoodIterator const& r) const { - return i_ < r.i_; - } - bool operator<=(NeighborhoodIterator const& r) const { - return i_ <= r.i_; - } - bool operator>(NeighborhoodIterator const& r) const { - return i_ > r.i_; - } - bool operator>=(NeighborhoodIterator const& r) const { - return i_ >= r.i_; - } - bool operator!=(const NeighborhoodIterator& r) const { - return i_ != r.i_; - } - bool operator==(const NeighborhoodIterator& r) const { - return i_ == r.i_; - } - - private: - neighborhood_type* neighborhood_ptr_; - index_type i_; - - value_type* pair_ptr_; - - void update_pair() { - if (pair_ptr_) { - delete pair_ptr_; - pair_ptr_ = nullptr; - } - if (i_ >= 0 && i_ < (index_type)neighborhood_ptr_->size()) { - pair_ptr_ = new value_type(neighborhood_ptr_->neighbors[i_], - neighborhood_ptr_->quadratic_biases[i_]); - } - } - - void swap(NeighborhoodIterator& other) noexcept { - std::swap(neighborhood_ptr_, other.neighborhood_ptr_); - std::swap(i_, other.i_); - std::swap(pair_ptr_, other.pair_ptr_); - } -}; - -template -class ConstNeighborhoodIterator { - public: - using bias_type = Bias; - using index_type = Index; - using neighborhood_type = Neighborhood; - - using value_type = std::pair; - using pointer = value_type*; - using reference = value_type&; - using difference_type = std::ptrdiff_t; - using iterator_category = std::random_access_iterator_tag; - - // empty constructor needed for Cython - ConstNeighborhoodIterator() - : neighborhood_ptr_(nullptr), i_(0), pair_ptr_(nullptr) {} - - // for the copy constructor, we want to copy the pointer to the - // neighborhood, but not to the pair_ptr_, since that's managed by each - // iterator separately - ConstNeighborhoodIterator( - const ConstNeighborhoodIterator& other) - : neighborhood_ptr_(other.neighborhood_ptr_), - i_(other.i_), - pair_ptr_(nullptr) { - update_pair(); - } - - // for the move constructor, we want to move everything - ConstNeighborhoodIterator( - ConstNeighborhoodIterator&& other) noexcept - : neighborhood_ptr_(nullptr), i_(0), pair_ptr_(nullptr) { - swap(other); - } - - ConstNeighborhoodIterator(const neighborhood_type* neighborhood, - index_type i) - : neighborhood_ptr_(neighborhood), i_(i), pair_ptr_(nullptr) { - update_pair(); - } - - ~ConstNeighborhoodIterator() { delete pair_ptr_; } - - ConstNeighborhoodIterator& operator=( - ConstNeighborhoodIterator const& other) { - if (this != &other) { - neighborhood_ptr_ = other.neighborhood_ptr_; - i_ = other.i_; - update_pair(); // handles pair_ptr_ - } - return *this; - } - - ConstNeighborhoodIterator& operator=( - ConstNeighborhoodIterator&& other) noexcept { - swap(other); - return *this; - } - - const reference operator*() const { return *pair_ptr_; } - - const pointer operator->() const { return pair_ptr_; } - - ConstNeighborhoodIterator& operator++() { - ++i_; - update_pair(); - return *this; - } - ConstNeighborhoodIterator& operator--() { - --i_; - update_pair(); - return *this; - } - ConstNeighborhoodIterator operator++(int) { - ConstNeighborhoodIterator r(*this); - ++i_; - update_pair(); - return r; - } - ConstNeighborhoodIterator operator--(int) { - ConstNeighborhoodIterator r(*this); - --i_; - update_pair(); - return r; - } - - ConstNeighborhoodIterator& operator+=(int n) { - i_ += n; - update_pair(); - return *this; - } - ConstNeighborhoodIterator& operator-=(int n) { - i_ -= n; - update_pair(); - return *this; - } - - ConstNeighborhoodIterator operator+(int n) const { - ConstNeighborhoodIterator r(*this); - return r += n; - } - ConstNeighborhoodIterator operator-(int n) const { - ConstNeighborhoodIterator r(*this); - return r -= n; - } - - difference_type operator-( - ConstNeighborhoodIterator const& r) const { - return i_ - r.i_; - } - - bool operator<(ConstNeighborhoodIterator const& r) const { - return i_ < r.i_; - } - bool operator<=(ConstNeighborhoodIterator const& r) const { - return i_ <= r.i_; - } - bool operator>(ConstNeighborhoodIterator const& r) const { - return i_ > r.i_; - } - bool operator>=(ConstNeighborhoodIterator const& r) const { - return i_ >= r.i_; - } - bool operator!=(const ConstNeighborhoodIterator& r) const { - return i_ != r.i_; - } - bool operator==(const ConstNeighborhoodIterator& r) const { - return i_ == r.i_; - } - - private: - const neighborhood_type* neighborhood_ptr_; - index_type i_; - - value_type* pair_ptr_; - - void update_pair() { - if (pair_ptr_) { - delete pair_ptr_; - pair_ptr_ = nullptr; - } - if (i_ >= 0 && i_ < (index_type)neighborhood_ptr_->size()) { - pair_ptr_ = new value_type(neighborhood_ptr_->neighbors[i_], - neighborhood_ptr_->quadratic_biases[i_]); - } - } - - void swap(ConstNeighborhoodIterator& other) noexcept { - std::swap(neighborhood_ptr_, other.neighborhood_ptr_); - std::swap(i_, other.i_); - std::swap(pair_ptr_, other.pair_ptr_); - } -}; - template class ConstQuadraticIterator { public: diff --git a/dimod/include/dimod/quadratic_model.h b/dimod/include/dimod/quadratic_model.h index cc2b31e27..d05518a57 100644 --- a/dimod/include/dimod/quadratic_model.h +++ b/dimod/include/dimod/quadratic_model.h @@ -153,12 +153,14 @@ class Neighborhood { /// Unsigned integral type that can represent non-negative values. using size_type = std::size_t; - /// A random access iterator to `pair`. - using iterator = NeighborhoodIterator; + /// Exactly `pair`. + using value_type = typename std::pair; - /// A random access iterator to `const pair.` - using const_iterator = ConstNeighborhoodIterator; + /// A random access iterator to `pair`. + using iterator = typename std::vector::iterator; + + /// A random access iterator to `const pair.` + using const_iterator = typename std::vector::const_iterator; /** * Return a reference to the bias associated with `v`. @@ -167,28 +169,27 @@ class Neighborhood { * neighborhood and throws a `std::out_of_range` exception if it is not. */ bias_type at(index_type v) const { - auto it = std::lower_bound(neighbors.begin(), neighbors.end(), v); - auto idx = std::distance(neighbors.begin(), it); - if (it != neighbors.end() && (*it) == v) { + auto it = this->lower_bound(v); + if (it != this->cend() && it->first == v) { // it exists - return quadratic_biases[idx]; + return it->second; } else { // it doesn't exist - throw std::out_of_range("given variables have no interaction"); + throw std::out_of_range("given variable has no interaction"); } } /// Returns an iterator to the beginning. - iterator begin() { return iterator(this, 0); } + iterator begin() { return this->neighborhood_.begin(); } /// Returns an iterator to the end. - iterator end() { return iterator(this, size()); } + iterator end() { return this->neighborhood_.end(); } /// Returns a const_iterator to the beginning. - const_iterator cbegin() const { return const_iterator(this, 0); } + const_iterator cbegin() const { return this->neighborhood_.cbegin(); } /// Returns a const_iterator to the end. - const_iterator cend() const { return const_iterator(this, size()); } + const_iterator cend() const { return this->neighborhood_.cend(); } /** * Insert a neighbor, bias pair at the end of the neighborhood. @@ -198,8 +199,7 @@ class Neighborhood { * last element. */ void emplace_back(index_type v, bias_type bias) { - neighbors.push_back(v); - quadratic_biases.push_back(bias); + this->neighborhood_.emplace_back(v, bias); } /** @@ -208,14 +208,10 @@ class Neighborhood { * Returns the number of element removed, either 0 or 1. */ size_type erase(index_type v) { - auto it = std::lower_bound(neighbors.begin(), neighbors.end(), v); - if (it != neighbors.end() && (*it) == v) { + auto it = this->lower_bound(v); + if (it != this->end() && it->first == v) { // is there to erase - auto idx = std::distance(neighbors.begin(), it); - - neighbors.erase(it); - quadratic_biases.erase(quadratic_biases.begin() + idx); - + this->neighborhood_.erase(it); return 1; } else { return 0; @@ -224,25 +220,17 @@ class Neighborhood { /// Erase elements from the neighborhood. void erase(iterator first, iterator last) { - auto start_dist = std::distance(begin(), first); - auto end_dist = std::distance(end(), last); - - quadratic_biases.erase(quadratic_biases.begin() + start_dist, - quadratic_biases.end() + end_dist); - neighbors.erase(neighbors.begin() + start_dist, - neighbors.end() + end_dist); + this->neighborhood_.erase(first, last); } /// Return an iterator to the first element that does not come before `v`. iterator lower_bound(index_type v) { - auto it = std::lower_bound(neighbors.begin(), neighbors.end(), v); - return iterator(this, std::distance(neighbors.begin(), it)); + return std::lower_bound(this->begin(), this->end(), v, this->cmp); } /// Return an iterator to the first element that does not come before `v`. const_iterator lower_bound(index_type v) const { - auto it = std::lower_bound(neighbors.cbegin(), neighbors.cend(), v); - return const_iterator(this, std::distance(neighbors.begin(), it)); + return std::lower_bound(this->cbegin(), this->cend(), v, this->cmp); } /** @@ -252,12 +240,13 @@ class Neighborhood { * than the size. */ size_type nbytes(bool capacity = false) const noexcept { + // so there is no guaruntee that the compiler will not implement + // pair as pointers or whatever, but this seems like a reasonable + // assumption. if (capacity) { - return this->neighbors.capacity() * sizeof(index_type) + - this->quadratic_biases.capacity() * sizeof(bias_type); + return this->neighborhood_.capacity() * sizeof(std::pair); } else { - return this->neighbors.size() * sizeof(index_type) + - this->quadratic_biases.size() * sizeof(bias_type); + return this->neighborhood_.size() * sizeof(std::pair); } } @@ -268,11 +257,11 @@ class Neighborhood { * the `value` provided without inserting `v`. */ bias_type get(index_type v, bias_type value = 0) const { - auto it = std::lower_bound(neighbors.begin(), neighbors.end(), v); - auto idx = std::distance(neighbors.begin(), it); - if (it != neighbors.end() && (*it) == v) { + auto it = this->lower_bound(v); + + if (it != this->cend() && it->first == v) { // it exists - return quadratic_biases[idx]; + return it->second; } else { // it doesn't exist return value; @@ -281,18 +270,15 @@ class Neighborhood { /// Request that the neighborhood capacity be at least enough to contain `n` /// elements. - void reserve(index_type n) { - neighbors.reserve(n); - quadratic_biases.reserve(n); - } + void reserve(index_type n) { this->neighborhood_.reserve(n); } /// Return the size of the neighborhood. - size_type size() const { return neighbors.size(); } + size_type size() const { return this->neighborhood_.size(); } /// Sort the neighborhood and sum the biases of duplicate variables. void sort_and_sum() { - if (!std::is_sorted(neighbors.begin(), neighbors.end())) { - utils::zip_sort(neighbors, quadratic_biases); + if (!std::is_sorted(this->begin(), this->end())) { + std::sort(this->begin(), this->end()); } // now remove any duplicates, summing the biases of duplicates @@ -300,56 +286,50 @@ class Neighborhood { size_type j = 1; // walk quickly through the neighborhood until we find a duplicate - while (j < neighbors.size() && neighbors[i] != neighbors[j]) { + while (j < this->neighborhood_.size() && + this->neighborhood_[i].first != this->neighborhood_[j].first) { ++i; ++j; } // if we found one, move into de-duplication - if (j < neighbors.size()) { - while (j < neighbors.size()) { - if (neighbors[i] == neighbors[j]) { - quadratic_biases[i] += quadratic_biases[j]; - ++j; - } else { - ++i; - neighbors[i] = neighbors[j]; - quadratic_biases[i] = quadratic_biases[j]; - ++j; - } + while (j < this->neighborhood_.size()) { + if (this->neighborhood_[i].first == this->neighborhood_[j].first) { + this->neighborhood_[i].second += this->neighborhood_[j].second; + ++j; + } else { + ++i; + this->neighborhood_[i] = this->neighborhood_[j]; + ++j; } - - // finally resize to contain only the unique values - neighbors.resize(i + 1); - quadratic_biases.resize(i + 1); } + + // finally resize to contain only the unique values + this->neighborhood_.resize(i + 1); } /** * Access the bias of `v`. * - * If `v` is in the neighborhood, the function returns a reference to its - * bias. If `v` is not in the neighborhood, it is inserted and a reference - * is returned to its bias. + * If `v` is in the neighborhood, the function returns a reference to + * its bias. If `v` is not in the neighborhood, it is inserted and a + * reference is returned to its bias. */ bias_type& operator[](index_type v) { - auto it = std::lower_bound(neighbors.begin(), neighbors.end(), v); - auto idx = std::distance(neighbors.begin(), it); - if (it == neighbors.end() || (*it) != v) { + auto it = this->lower_bound(v); + if (it == this->end() || it->first != v) { // it doesn't exist so insert - neighbors.insert(it, v); - quadratic_biases.insert(quadratic_biases.begin() + idx, 0); + it = this->neighborhood_.emplace(it, v, 0); } - - return quadratic_biases[idx]; + return it->second; } protected: - std::vector neighbors; - std::vector quadratic_biases; + std::vector neighborhood_; - friend class NeighborhoodIterator; - friend class ConstNeighborhoodIterator; + static inline bool cmp(value_type ub, index_type v) { + return ub.first < v; + } }; template diff --git a/releasenotes/notes/vector-of-pairs-a91dc26f70ae8947.yaml b/releasenotes/notes/vector-of-pairs-a91dc26f70ae8947.yaml new file mode 100644 index 000000000..8c9efc41c --- /dev/null +++ b/releasenotes/notes/vector-of-pairs-a91dc26f70ae8947.yaml @@ -0,0 +1,9 @@ +--- +upgrade: + - | + Packages that require binary compatibility with dimod and that were compiled + with 0.10.0 will not work with 0.11.0. +fixes: + - | + Fixes performance regression on energy calculations introduced in 0.10.0. + See `#1025 `_ diff --git a/tests/test_bqm.py b/tests/test_bqm.py index d8b74d15a..db8d1af03 100644 --- a/tests/test_bqm.py +++ b/tests/test_bqm.py @@ -1598,7 +1598,7 @@ def test_small(self, name, BQM): size = sum([itemsize, # offset bqm.num_variables*itemsize, # linear - 2*bqm.num_interactions*(itemsize + np.dtype(np.float32).itemsize), # quadratic + 2*bqm.num_interactions*(2*itemsize), # quadratic ]) self.assertEqual(bqm.nbytes(), size) diff --git a/tests/test_quadratic_model.py b/tests/test_quadratic_model.py index 01f5c7b12..215873296 100644 --- a/tests/test_quadratic_model.py +++ b/tests/test_quadratic_model.py @@ -631,7 +631,7 @@ def test_small(self, dtype): size = sum([itemsize, # offset qm.num_variables*itemsize, # linear - 2*qm.num_interactions*(itemsize + np.dtype(np.float32).itemsize), # quadratic + 2*qm.num_interactions*(2*itemsize), # quadratic qm.num_variables*3*itemsize, # vartype info and bounds ]) diff --git a/testscpp/tests/test_quadratic_model.cpp b/testscpp/tests/test_quadratic_model.cpp index 69471446e..f019cab41 100644 --- a/testscpp/tests/test_quadratic_model.cpp +++ b/testscpp/tests/test_quadratic_model.cpp @@ -1007,11 +1007,17 @@ TEMPLATE_TEST_CASE( bqm.add_quadratic(1, 2, 1.5); bqm.add_quadratic(2, 3, 1.5); + auto pair_size = sizeof(std::pair); + + // this assumption is not guaranteed across compilers, but it + // is required for this test to make sense + CHECK(pair_size == 2 * sizeof(TestType)); + THEN("we can determine the number of bytes used by the elements") { CHECK(bqm.nbytes() == - (bqm.num_variables() + 2 * bqm.num_interactions()) * - sizeof(TestType) + - 2 * bqm.num_interactions() * sizeof(int) + sizeof(TestType)); + bqm.num_variables() * sizeof(TestType) // linear + + 2 * bqm.num_interactions() * pair_size // quadratic + + sizeof(TestType)); // offset CHECK(bqm.nbytes(true) >= bqm.nbytes()); } @@ -1020,12 +1026,13 @@ TEMPLATE_TEST_CASE( THEN("we can determine the number of bytes used by the elements") { CHECK(qm.nbytes() == - (qm.num_variables() + 2 * qm.num_interactions()) * - sizeof(TestType) + - 2 * qm.num_interactions() * sizeof(int) + - qm.num_variables() * - sizeof(dimod::VarInfo) + - sizeof(TestType)); + qm.num_variables() * sizeof(TestType) // linear + + 2 * qm.num_interactions() * + pair_size // quadratic + + sizeof(TestType) // offset + + qm.num_variables() * + sizeof(dimod::VarInfo< + TestType>)); // vartypes CHECK(qm.nbytes(true) >= qm.nbytes()); } }