fix(op): compare NormalOperatorSequence against the other operand

[evaleev]

NormalOperatorSequence::static_equal compared the underlying vector against itself instead of against that_cast:

if (this->hash_value() == that.hash_value())
  return static_cast<const base_type &>(*this) ==
         static_cast<const base_type &>(*this);   // <-- *this twice

so any two NormalOperatorSequences sharing a vacuum compared equal at the Expr level. 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 passes. The if (this->empty()) return true; early return was wrong for the same reason — it never checked whether the other operand was empty.

The concrete operator== was fine, so the two comparison paths disagreed with each other.

Fix

• static_equal now delegates to the concrete operator==, matching Operator::static_equal and NormalOperator::static_equal.
• operator== now also compares the vacuum, so both paths agree on empty sequences — for an empty sequence the vacuum is the only distinguishing state. For non-empty sequences this is redundant (check_vacuum enforces uniformity and NormalOperator::operator== already compares vacuum).
• operator== no longer slices both operands into base_type by value just to compare them.

Impact

Reachable today via any Expr-level comparison of two NormalOperatorSequence objects. Note that #589 widens the exposure: it gives NormalOperatorSequence a clone(), and Sum::append/Product::append call factor->clone(), so these objects can be stored into expressions — and thus fed to simplify, add_identical, and canonicalization — where before clone() threw. This PR is independent of #589 and branches from master.

Tests

New SECTION("equality") in tests/unit/test_op.cpp, verified to fail before the fix (5 assertions) and pass after. Full unit suite green (62 test cases).

[Copilot]

Pull request overview

Fixes incorrect Expr-level equality for NormalOperatorSequence by ensuring comparisons actually compare against the other operand (and by aligning static_equal with the concrete operator== behavior), with unit coverage added to prevent regressions.

Changes:

• Update NormalOperatorSequence::static_equal to delegate to the concrete operator== implementation.
• Update NormalOperatorSequence::operator== to compare vacuums and avoid by-value slicing of the underlying container.
• Add a new unit-test section validating both concrete and Expr-level equality, including empty-sequence vacuum distinctions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
SeQuant/core/op.hpp	Fixes NormalOperatorSequence equality logic and aligns Expr-level and concrete comparison paths.
tests/unit/test_op.cpp	Adds regression tests covering concrete vs Expr-level equality, including empty-sequence vacuum behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[evaleev]

Good catch — confirmed and fixed in 2038669.

check_vacuum() only assigned vacuum_ when size() > 0, so an empty sequence built through any of the three list/pack constructors kept the Vacuum::Physical member default, while a default-constructed one took the vacuum from the context. Under the default test context (SingleProduct) those two empty sequences compared unequal.

The fix puts the empty-sequence vacuum in one place: the default constructor now routes through check_vacuum() as well, and check_vacuum() always assigns, falling back to get_default_context(S).vacuum() when there is no constituent operator to read it from. Four assertions covering this were added to SECTION("equality") and verified to fail before the change.