From 898d94877f9a23af2475869d71925e1a1e51c2a5 Mon Sep 17 00:00:00 2001 From: Eduard Valeyev Date: Thu, 13 Aug 2026 13:47:20 -0400 Subject: [PATCH 1/2] fix(op): compare NormalOperatorSequence against the other operand NormalOperatorSequence::static_equal compared the underlying vector against itself rather than against `that_cast`, so any two sequences sharing a vacuum compared equal. The hash guard did not contain the damage: NormalOperatorSequence does not override Expr::memoizing_hash, so hash_value() is always 0 and the guard always passed. The `this->empty()` early return was wrong for the same reason -- it ignored whether the other operand was empty too. Delegate to the concrete operator==, matching Operator::static_equal and NormalOperator::static_equal, and teach that operator== to compare the vacuum as well, so the two comparison paths agree on empty sequences (whose vacuum is their only distinguishing state). Also stop slicing both operands into base_type by value while comparing them. --- SeQuant/core/op.hpp | 14 ++++---------- tests/unit/test_op.cpp | 40 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/SeQuant/core/op.hpp b/SeQuant/core/op.hpp index e38241e269..9845806253 100644 --- a/SeQuant/core/op.hpp +++ b/SeQuant/core/op.hpp @@ -1063,7 +1063,9 @@ class NormalOperatorSequence : public container::svector>, friend bool operator==(const NormalOperatorSequence &nopseq1, const NormalOperatorSequence &nopseq2) { - return static_cast(nopseq1) == static_cast(nopseq2); + return nopseq1.vacuum() == nopseq2.vacuum() && + static_cast(nopseq1) == + static_cast(nopseq2); } private: @@ -1089,15 +1091,7 @@ class NormalOperatorSequence : public container::svector>, bool static_equal(const Expr &that) const override { const auto &that_cast = static_cast(that); - if (this->vacuum() == that_cast.vacuum()) { - if (this->empty()) return true; - if (this->hash_value() == that.hash_value()) - return static_cast(*this) == - static_cast(*this); - else - return false; - } else - return false; + return *this == that_cast; } }; diff --git a/tests/unit/test_op.cpp b/tests/unit/test_op.cpp index f6320d3f6d..82cbed146e 100644 --- a/tests/unit/test_op.cpp +++ b/tests/unit/test_op.cpp @@ -134,6 +134,44 @@ TEST_CASE("op", "[elements]") { FNOperator(cre({L"i_5"}), ann({L"i_6"}))})); } + SECTION("equality") { + const FNOperatorSeq nopseq1({FNOperator(cre({L"i_1"}), ann({L"i_2"}))}); + const FNOperatorSeq nopseq2({FNOperator(cre({L"i_3"}), ann({L"i_4"}))}); + const FNOperatorSeq nopseq12({FNOperator(cre({L"i_1"}), ann({L"i_2"})), + FNOperator(cre({L"i_3"}), ann({L"i_4"}))}); + const FNOperatorSeq nopseq_empty; + + REQUIRE(nopseq1 == + FNOperatorSeq({FNOperator(cre({L"i_1"}), ann({L"i_2"}))})); + REQUIRE_FALSE(nopseq1 == nopseq2); + REQUIRE_FALSE(nopseq1 == nopseq12); + REQUIRE_FALSE(nopseq1 == nopseq_empty); + + // Expr-level comparison must agree with the concrete comparison, i.e. it + // must compare the operators, not just the vacuum + const Expr &expr1 = nopseq1; + const Expr &expr2 = nopseq2; + const Expr &expr12 = nopseq12; + const Expr &expr_empty = nopseq_empty; + + CHECK(expr1 == expr1); + CHECK_FALSE(expr1 == expr2); + CHECK_FALSE(expr1 == expr12); + CHECK_FALSE(expr1 == expr_empty); + CHECK_FALSE(expr_empty == expr1); + + // empty sequences differing only in vacuum are not equal + const FNOperatorSeq nopseq_empty_physical = [] { + Context ctx = get_default_context(Statistics::FermiDirac); + ctx.set(Vacuum::Physical); + auto resetter = set_scoped_default_context(ctx); + return FNOperatorSeq{}; + }(); + REQUIRE(nopseq_empty.vacuum() != nopseq_empty_physical.vacuum()); + CHECK_FALSE(nopseq_empty == nopseq_empty_physical); + CHECK_FALSE(expr_empty == static_cast(nopseq_empty_physical)); + } + SECTION("adjoint") { auto o1 = adjoint(FOp(Index(L"i_1"), Action::Create)); REQUIRE(o1.statistics == Statistics::FermiDirac); @@ -289,7 +327,7 @@ TEST_CASE("op", "[elements]") { ann({Index{L"a_1", {L"i_1", L"i_2"}}, Index{L"a_2", {L"i_1", L"i_2"}}, Index{L"a_1", {L"i_1", L"i_3"}}, Index{L"a_4"}})); REQUIRE_NOTHROW(nop1.hug()); - auto& hug1 = nop1.hug(); + auto &hug1 = nop1.hug(); REQUIRE(hug1->num_edges() == 8); REQUIRE(hug1->num_groups() == 5); REQUIRE(hug1->num_nonempty_groups() == 5); From 20386697e62b20e67708e4ccdbf7a7e3ac359fd0 Mon Sep 17 00:00:00 2001 From: Eduard Valeyev Date: Thu, 13 Aug 2026 14:58:17 -0400 Subject: [PATCH 2/2] fix(op): give every empty NormalOperatorSequence the context vacuum Now that operator== gates on vacuum(), the vacuum_ invariant is load-bearing in a second place, and check_vacuum() did not establish it for empty sequences: it only assigned vacuum_ when size() > 0, leaving the Vacuum::Physical member default in place. So a sequence built empty through any of the three list/pack constructors disagreed with a default-constructed one, which takes its vacuum from the context. Route the default constructor through check_vacuum() too and have check_vacuum() always assign, falling back to the default context's vacuum when there is no constituent operator to read it from. That puts the empty-sequence vacuum in exactly one place. Reported by Copilot on #590. --- SeQuant/core/op.hpp | 11 +++++++---- tests/unit/test_op.cpp | 12 ++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/SeQuant/core/op.hpp b/SeQuant/core/op.hpp index 9845806253..5225429122 100644 --- a/SeQuant/core/op.hpp +++ b/SeQuant/core/op.hpp @@ -1001,7 +1001,7 @@ class NormalOperatorSequence : public container::svector>, using base_type::operator[]; /// constructs an empty sequence - NormalOperatorSequence() : vacuum_(get_default_context(S).vacuum()) {} + NormalOperatorSequence() { check_vacuum(); } /// constructs from a parameter pack template >, private: Vacuum vacuum_ = Vacuum::Physical; /// ensures that all operators use same vacuum, and sets vacuum_ + /// @note an empty sequence has no constituent operator to take the vacuum + /// from, hence it uses the default context's vacuum; every constructor + /// must call this so that empty sequences agree on their vacuum, which + /// is their only distinguishing state (@sa operator==) void check_vacuum() { const bool all_same_vaccum = std::ranges::adjacent_find(*this, std::ranges::not_equal_to{}, @@ -1084,9 +1088,8 @@ class NormalOperatorSequence : public container::svector>, "NormalOperator objects to use same vacuum"); } - if (size() > 0) { - vacuum_ = this->cbegin()->vacuum(); - } + vacuum_ = + size() > 0 ? this->cbegin()->vacuum() : get_default_context(S).vacuum(); } bool static_equal(const Expr &that) const override { diff --git a/tests/unit/test_op.cpp b/tests/unit/test_op.cpp index 82cbed146e..a0dc64550f 100644 --- a/tests/unit/test_op.cpp +++ b/tests/unit/test_op.cpp @@ -170,6 +170,18 @@ TEST_CASE("op", "[elements]") { REQUIRE(nopseq_empty.vacuum() != nopseq_empty_physical.vacuum()); CHECK_FALSE(nopseq_empty == nopseq_empty_physical); CHECK_FALSE(expr_empty == static_cast(nopseq_empty_physical)); + + // ... but an empty sequence takes its vacuum from the default context + // regardless of which constructor produced it + const FNOperatorSeq nopseq_empty_from_nops( + std::initializer_list{}); + const FNOperatorSeq nopseq_empty_from_ops(std::initializer_list{}); + REQUIRE(nopseq_empty_from_nops.empty()); + REQUIRE(nopseq_empty_from_ops.empty()); + CHECK(nopseq_empty_from_nops.vacuum() == nopseq_empty.vacuum()); + CHECK(nopseq_empty_from_ops.vacuum() == nopseq_empty.vacuum()); + CHECK(nopseq_empty == nopseq_empty_from_nops); + CHECK(nopseq_empty == nopseq_empty_from_ops); } SECTION("adjoint") {