From d7a947532124415cd7f239b985f9f5c332b69165 Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Mon, 2 Mar 2026 14:20:39 +0100 Subject: [PATCH 1/3] Reduce indentation level --- SeQuant/core/utility/permutation.hpp | 40 +++++++++++++++------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/SeQuant/core/utility/permutation.hpp b/SeQuant/core/utility/permutation.hpp index 0a3ad424b1..8e49f4d26f 100644 --- a/SeQuant/core/utility/permutation.hpp +++ b/SeQuant/core/utility/permutation.hpp @@ -50,33 +50,35 @@ std::size_t count_cycles(Seq0&& v0, const Seq1& v1) { std::size_t n_cycles = 0; for (auto it = v.begin(); it != v.end(); ++it) { - if (*it != null) { - n_cycles++; + if (*it == null) { + continue; + } - auto idx = std::distance(v.begin(), it); - SEQUANT_ASSERT(idx >= 0); + n_cycles++; - auto it0 = it; + auto idx = std::distance(v.begin(), it); + SEQUANT_ASSERT(idx >= 0); - auto it1 = std::find(v1.begin(), v1.end(), *it0); - SEQUANT_ASSERT(it1 != v1.end()); + auto it0 = it; - auto idx1 = std::distance(v1.begin(), it1); - SEQUANT_ASSERT(idx1 >= 0); + auto it1 = std::find(v1.begin(), v1.end(), *it0); + SEQUANT_ASSERT(it1 != v1.end()); + + auto idx1 = std::distance(v1.begin(), it1); + SEQUANT_ASSERT(idx1 >= 0); - do { - it0 = std::find(v.begin(), v.end(), v[idx1]); - SEQUANT_ASSERT(it0 != v.end()); + do { + it0 = std::find(v.begin(), v.end(), v[idx1]); + SEQUANT_ASSERT(it0 != v.end()); - it1 = std::find(v1.begin(), v1.end(), *it0); - SEQUANT_ASSERT(it1 != v1.end()); + it1 = std::find(v1.begin(), v1.end(), *it0); + SEQUANT_ASSERT(it1 != v1.end()); - idx1 = std::distance(v1.begin(), it1); - SEQUANT_ASSERT(idx1 >= 0); + idx1 = std::distance(v1.begin(), it1); + SEQUANT_ASSERT(idx1 >= 0); - *it0 = null; - } while (idx1 != idx); - } + *it0 = null; + } while (idx1 != idx); } return n_cycles; }; From 4f035e46ddc017db8c2277e014d03a3775e5c71e Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Mon, 2 Mar 2026 14:33:06 +0100 Subject: [PATCH 2/3] Don't modify range in count_cycles (and allow for general range inputs) Fixes #503 --- SeQuant/core/utility/permutation.hpp | 69 +++++++++++++--------------- 1 file changed, 31 insertions(+), 38 deletions(-) diff --git a/SeQuant/core/utility/permutation.hpp b/SeQuant/core/utility/permutation.hpp index 8e49f4d26f..d3f25cb211 100644 --- a/SeQuant/core/utility/permutation.hpp +++ b/SeQuant/core/utility/permutation.hpp @@ -1,17 +1,15 @@ #ifndef SEQUANT_PERMUTATION_HPP #define SEQUANT_PERMUTATION_HPP +#include #include #include -#include - #include #include #include +#include #include -#include -#include namespace sequant { @@ -21,63 +19,58 @@ namespace sequant { /// by stacking \p v0 and \p v1 on top of each other. /// @tparam Seq0 (reference to) a container type /// @tparam Seq1 (reference to) a container type -/// @param v0 first sequence; if passed as an rvalue reference, it is moved from -/// @param[in] v1 second sequence +/// @param v0 first range +/// @param v1 second range /// @pre \p v0 is a permutation of \p v1 /// @return the number of cycles template -std::size_t count_cycles(Seq0&& v0, const Seq1& v1) { - std::remove_reference_t v(std::forward(v0)); - using T = std::decay_t; - SEQUANT_ASSERT(ranges::is_permutation(v, v1)); +std::size_t count_cycles(Seq0&& v0, Seq1&& v1) { + using std::ranges::begin; + using std::ranges::end; + using std::ranges::size; + SEQUANT_ASSERT(std::ranges::is_permutation(v0, v1)); // This function can't deal with duplicate entries in v0 or v1 - SEQUANT_ASSERT(std::set(std::begin(v0), std::end(v0)).size() == v0.size()); - SEQUANT_ASSERT(std::set(std::begin(v1), std::end(v1)).size() == v1.size()); - - auto make_null = []() -> T { - if constexpr (std::is_arithmetic_v) { - return -1; - } else if constexpr (std::is_same_v) { - return L"p_50"; - } + SEQUANT_ASSERT(std::set(begin(v0), end(v0)).size() == size(v0)); + SEQUANT_ASSERT(std::set(begin(v1), end(v1)).size() == size(v1)); - SEQUANT_UNREACHABLE; - }; - - const auto null = make_null(); - SEQUANT_ASSERT(ranges::contains(v, null) == false); - SEQUANT_ASSERT(ranges::contains(v1, null) == false); + container::set visited; + visited.reserve(size(v0)); std::size_t n_cycles = 0; - for (auto it = v.begin(); it != v.end(); ++it) { - if (*it == null) { + for (auto it = begin(v0); it != end(v0); ++it) { + if (visited.find(it) != end(visited)) { continue; } n_cycles++; - auto idx = std::distance(v.begin(), it); + auto idx = std::distance(begin(v0), it); SEQUANT_ASSERT(idx >= 0); auto it0 = it; - auto it1 = std::find(v1.begin(), v1.end(), *it0); - SEQUANT_ASSERT(it1 != v1.end()); + auto it1 = std::find(begin(v1), end(v1), *it0); + SEQUANT_ASSERT(it1 != end(v1)); - auto idx1 = std::distance(v1.begin(), it1); + auto idx1 = std::distance(begin(v1), it1); SEQUANT_ASSERT(idx1 >= 0); do { - it0 = std::find(v.begin(), v.end(), v[idx1]); - SEQUANT_ASSERT(it0 != v.end()); + auto val_it = begin(v0); + std::advance(val_it, idx1); + SEQUANT_ASSERT(val_it != end(v0)); + + it0 = std::find(begin(v0), end(v0), *val_it); + SEQUANT_ASSERT(it0 != end(v0)); - it1 = std::find(v1.begin(), v1.end(), *it0); - SEQUANT_ASSERT(it1 != v1.end()); + it1 = std::find(begin(v1), end(v1), *it0); + SEQUANT_ASSERT(it1 != end(v1)); - idx1 = std::distance(v1.begin(), it1); + idx1 = std::distance(begin(v1), it1); SEQUANT_ASSERT(idx1 >= 0); - *it0 = null; + SEQUANT_ASSERT(visited.find(it0) == end(visited)); + visited.insert(it0); } while (idx1 != idx); } return n_cycles; @@ -107,7 +100,7 @@ int permutation_parity(std::span p, bool overwrite = false) { } if (overwrite) { - ranges::for_each(p, [N](auto& e) { e -= N; }); + std::ranges::for_each(p, [N](auto& e) { e -= N; }); } return parity; From b69289a40d664657512963c7652413636a9f9378 Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Fri, 6 Mar 2026 10:30:04 +0100 Subject: [PATCH 3/3] Make count_cycles impl more efficient (and compact) --- SeQuant/core/utility/permutation.hpp | 57 +++++++++++++--------------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/SeQuant/core/utility/permutation.hpp b/SeQuant/core/utility/permutation.hpp index d3f25cb211..a440ad60a4 100644 --- a/SeQuant/core/utility/permutation.hpp +++ b/SeQuant/core/utility/permutation.hpp @@ -9,7 +9,6 @@ #include #include #include -#include namespace sequant { @@ -33,46 +32,44 @@ std::size_t count_cycles(Seq0&& v0, Seq1&& v1) { SEQUANT_ASSERT(std::set(begin(v0), end(v0)).size() == size(v0)); SEQUANT_ASSERT(std::set(begin(v1), end(v1)).size() == size(v1)); - container::set visited; - visited.reserve(size(v0)); + container::svector visited; + visited.resize(size(v0), false); std::size_t n_cycles = 0; - for (auto it = begin(v0); it != end(v0); ++it) { - if (visited.find(it) != end(visited)) { + std::size_t start_col = 0; + for (auto it = begin(v0); it != end(v0); ++it, ++start_col) { + if (visited[start_col]) { + // This column has already been part of a previous cycle continue; } n_cycles++; - auto idx = std::distance(begin(v0), it); - SEQUANT_ASSERT(idx >= 0); - + std::size_t current_col = 0; auto it0 = it; - - auto it1 = std::find(begin(v1), end(v1), *it0); - SEQUANT_ASSERT(it1 != end(v1)); - - auto idx1 = std::distance(begin(v1), it1); - SEQUANT_ASSERT(idx1 >= 0); - do { - auto val_it = begin(v0); - std::advance(val_it, idx1); - SEQUANT_ASSERT(val_it != end(v0)); - - it0 = std::find(begin(v0), end(v0), *val_it); - SEQUANT_ASSERT(it0 != end(v0)); - - it1 = std::find(begin(v1), end(v1), *it0); - SEQUANT_ASSERT(it1 != end(v1)); + // Find corresponding element in v1 + auto it1 = std::ranges::find(v1, *it0); + SEQUANT_ASSERT(std::distance(begin(v1), it1) >= 0); + + // Determine column of the determined corresponding element + current_col = static_cast(std::distance(begin(v1), it1)); + SEQUANT_ASSERT(current_col < size(v0)); + + // Set it0 to the element in the determined column in v0 + it0 = begin(v0); + std::advance(it0, current_col); + + // Mark current_col as visited + SEQUANT_ASSERT(!visited[current_col]); + visited[current_col] = true; + } while (start_col != current_col); + } - idx1 = std::distance(begin(v1), it1); - SEQUANT_ASSERT(idx1 >= 0); + // All columns must have been visited (otherwise, we'll have missed + // at least one cycle) + SEQUANT_ASSERT(std::ranges::all_of(visited, [](bool val) { return val; })); - SEQUANT_ASSERT(visited.find(it0) == end(visited)); - visited.insert(it0); - } while (idx1 != idx); - } return n_cycles; };