Skip to content

Source default tensor symmetries from the Context - #557

Merged
evaleev merged 11 commits into
default-tensor-symmetryfrom
default-tensor-symmetry-context
Aug 23, 2026
Merged

Source default tensor symmetries from the Context#557
evaleev merged 11 commits into
default-tensor-symmetryfrom
default-tensor-symmetry-context

Conversation

@evaleev

@evaleev evaleev commented Jun 16, 2026

Copy link
Copy Markdown
Member

Stacked on top of #553 (base branch default-tensor-symmetry); this PR shows only the incremental changes on top of it.

Motivation

#553 makes tensors non-symmetric by default, but hardcodes the defaults and leaves the deserializer's column default (Nonsymm) inconsistent with the Tensor ctor's (Symm). That inconsistency made particle-symmetric amplitudes parse as column-Nonsymm (so they canonicalized differently and no longer merged), and broke ~18 unit + 5 integration tests.

Approach

Make the default tensor symmetries a property of the active sequant::Context, so the library default is the safest (fully non-symmetric) while applications fine-tune for ergonomics.

  • Core Context gains symmetry, hermiticity, and column_symmetry defaults (Defaults + Options + accessors + set()). Library defaults: Nonsymm / NonHermitian / Nonsymm.
  • BraKet symmetry is never a Context knob — it is a derived property, computed per-tensor from the default Hermiticity and base_field, consistent with the earlier removal of Context::braket_symmetry (8c4669d).
  • Tensor ctors are deliberately Context-independent. Unspecified perm/column/hermiticity resolve against the fixed library defaults in Tensor::Defaults, never against the active Context, so the meaning of a ctor call does not depend on ambient global state (and the hot path stays lock-free). Only the deserializer honors the Context-level symmetry defaults, since it is the boundary that turns under-specified external input into tensors; domains needing a different programmatic default (e.g. mbpt, whose tensors are particle/column-symmetric) pass it explicitly. The ctors also gain a TensorSymmetries named-parameter pack, so such a per-domain default can be spelled once per TU and reused at every construction site.
  • Deserializer default symmetries are sourced from the active Context.
  • Context::operator== now compares the new symmetry defaults (otherwise set_scoped_default_context would silently refuse to switch a context that differs only in these — a latent bug surfaced by the new tests).
  • mbpt applications (unit-test harness + srcc/stcc integration) opt into the particle-symmetric convention via column = Symm.
  • Test expectations updated to the resulting (physically correct) symmetries; new tests cover the Context-driven defaults and the rejection of braket symmetry on (anti)symmetrization operators.

Testing

Built with the full debug config (clang-17, AddressSanitizer, Unity, ccache, all eval backends: TiledArray/BTAS/TAPP/Torch).

  • unit_tests-sequant: 7069 assertions / 70 cases pass.
  • check-sequant: all tests pass except sequant/unit/python/basic/run, which aborts at AddressSanitizer init (ASan-instrumented module dlopened into a non-ASan Python on macOS) — an environment limitation, unrelated to these changes.
  • All changed files pass clang-format-17 --Werror.

Builds on #553 ("non-symmetric by default") by making the default tensor
symmetries configurable through sequant::Context rather than hardcoding them.

- Context gains default `symmetry`, `hermiticity`, and `column_symmetry`
  (Defaults + Options + accessors + set()). Library defaults are the safest
  (Nonsymm / NonHermitian / Nonsymm). BraKet symmetry is never a Context knob:
  it is a derived property, computed per-tensor from the default Hermiticity
  and base_field, consistent with the removal of Context::braket_symmetry.
- Tensor ctors resolve unspecified perm/column/hermiticity against the active
  Context; the Context is consulted only when something is unspecified, so
  fully-explicit construction (the hot path) stays lock-free.
- The deserializer's default symmetries are sourced from the active Context.
- Context::operator== now also compares the symmetry defaults, so scoped/default
  context switches that differ only in these actually take effect.
- mbpt applications (unit-test harness + srcc/stcc integration) opt into the
  particle-symmetric convention via column = Symm.
- Update unit/integration expectations to the resulting (physically correct)
  symmetries; add tests for the Context-driven defaults and for rejecting
  braket symmetry on (anti)symmetrization operators.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR moves the “default tensor symmetry” behavior out of hardcoded defaults and into the active sequant::Context, so that library defaults are conservative (fully non-symmetric / non-Hermitian / non-column-symmetric) while applications can opt into more ergonomic conventions (e.g., particle/column symmetry for MBPT). It also updates deserialization and tests to reflect Context-sourced defaults and the updated canonicalization/ordering behavior.

Changes:

  • Add Context-level defaults for Symmetry, Hermiticity, and ColumnSymmetry, and include them in Context::operator==.
  • Update Tensor construction to resolve unspecified symmetry attributes against the active Context (deriving BraKetSymmetry from Hermiticity + base field).
  • Update v1 deserialization to source default symmetries from the active Context, and adjust unit/integration tests to match the new defaults and ordering.

Reviewed changes

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

Show a summary per file
File Description
SeQuant/core/context.hpp Adds Context defaults + Options fields + accessors/setters for symmetry/hermiticity/column symmetry.
SeQuant/core/context.cpp Wires new Options fields into construction, adds accessors/setters, and updates operator== to compare new defaults.
SeQuant/core/expressions/tensor.hpp Introduces Context-driven defaulting via resolve_symmetries() and updates constructors accordingly.
SeQuant/core/io/serialization/v1/deserialize.cpp Sources deserializer default symmetries from get_default_context().
SeQuant/core/io/serialization/v1/ast_conversions.hpp Clarifies reserved (anti)symmetrizer symmetry handling; converts AST symmetries into Tensor construction.
tests/unit/test_main.cpp Sets test default Context to column_symmetry = Symm for MBPT-oriented expectations.
tests/unit/test_tensor.cpp Updates braket/adjoint expectations and adds coverage for Context-driven defaults.
tests/unit/test_parse.cpp Adds tests for rejecting braket symmetry on reserved (anti)symmetrizers.
tests/unit/test_wick.cpp Updates latex expectation due to changed canonical ordering under new defaults.
tests/unit/test_tensor_network.cpp Updates expected index ordering and adjusts a deserialize string to rely on Context defaults.
tests/unit/test_spin.cpp Updates expected serialized symmetry specifiers to match new defaulting/derivation behavior.
tests/unit/test_mbpt.cpp Updates expected strings to reflect Context-driven defaults and explicit symmetry tags.
tests/unit/test_canonicalize.cpp Updates deserialize fixtures to omit now-defaulted braket specifiers for Ŝ where appropriate.
tests/unit/test_biorthogonalization.cpp Removes per-call def_col_symm overrides, relying on Context defaults instead.
tests/unit/data/sf_r2_direct_real_inc.hpp Updates a large expected fixture string to match updated defaulting/serialization behavior.
tests/integration/srcc.cpp Sets MBPT integration Context default column_symmetry = Symm.
tests/integration/stcc.cpp Sets MBPT integration Context default column_symmetry = Symm.
Comments suppressed due to low confidence (1)

SeQuant/core/io/serialization/v1/ast_conversions.hpp:337

  • Deserializing reserved (anti)symmetrization operators (Ŝ/Â) currently inherits the Context-driven default Hermiticity when no explicit braket specifier is provided. If the active Context defaults to Hermitian, this will derive a non-Nonsymm braket symmetry and make plain "Ŝ{...}" / "Â{...}" fail to parse even though the default form should be accepted. These operators should ignore Context braket defaults and remain braket-Nonsymm unless the user explicitly requests otherwise (in which case the ctor should reject it).
    if (tensor.name == reserved::antisymm_label()) {
      perm_symm = Symmetry::Antisymm;
    } else if (tensor.name == reserved::symm_label()) {
      column_symm = ColumnSymmetry::Symm;
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread SeQuant/core/expressions/tensor.hpp Outdated
…ss of Context

Addresses Copilot review on #557:
- A bare "Ŝ{...}"/"Â{...}" previously inherited the Context's default
  Hermiticity, so under a Hermitian-default Context it would derive a
  non-Nonsymm braket and fail to construct. Force braket = Nonsymm for these
  reserved operators when no braket spec is given (an explicit spec is left for
  the Tensor ctor to reject). Covered by a new test under a Hermitian Context.
- Fix a misleading comment in Tensor::resolve_symmetries() about when the
  resolved Hermiticity is used.
@evaleev

evaleev commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

Also addressed the low-confidence note on ast_conversions.hpp (reserved (anti)symmetrizers inheriting the Context braket default): that was a real latent bug — under a Hermitian-default Context a plain Ŝ{...}/Â{...} would derive a non-Nonsymm braket and fail to construct. Fixed in 7fdb794 by forcing braket = Nonsymm for these operators when no braket spec is given (an explicit spec is still left for the Tensor ctor to reject), with a new regression test parsing them under a Hermitian Context.

@evaleev
evaleev requested a review from Krzmbrzl June 16, 2026 04:50
Programmatic Tensor construction now resolves unspecified symmetry
attributes against fixed library defaults instead of the active Context;
only deserialization honors the Context-level symmetry/hermiticity/
column-symmetry defaults. This makes a ctor call's meaning independent of
ambient global state.

The fixed defaults are the safest fully non-symmetric / non-Hermitian
choice (Symmetry::Nonsymm, Hermiticity::NonHermitian,
ColumnSymmetry::Nonsymm), encoded once in the new public Tensor::Defaults
struct and consumed by resolve_symmetries(). Ctor @PARAM docs now state
which value each unspecified optional resolves to. Adds a named-parameter
idiom (TensorSymmetries, C++20 designated initializers) so a TU can define
a symmetry pack once and reuse it, sidestepping positional-arg strictness.

context.hpp docs retargeted to describe the deserializer-only role of the
Context symmetry knobs.

MBPT/physics sites, whose particles are indistinguishable (hence tensors
are particle/column-symmetric), now pass ColumnSymmetry::Symm explicitly:
op.cpp, spin.cpp, antisymmetrizer.cpp, biorthogonalization.cpp, and the
DF/THC factor builders (rules/df.cpp, rules/thc.cpp).

Unit tests updated to the new defaults: particle-symmetric constructions
carry an explicit column-Symm pack (per-TU TensorSymmetries ps), matching
the parsed references that still follow the Context. Full unit suite green
(6563 assertions, 49 cases).
@evaleev

evaleev commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

@Krzmbrzl this should be combining #553 with Context-controllable defaults for deserialize ... plus new (better) named-param ctors for Tensor

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.

Comment thread SeQuant/core/expressions/tensor.hpp Outdated
Comment on lines +430 to +434
: Tensor(
std::forward<S>(label), std::move(bra_indices),
std::move(ket_indices), std::move(aux_indices), reserved_tag{},
resolve_symmetries(s, bks_opt, ps, herm_opt,
sequant::base_field(bra_indices, ket_indices))) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is safe as written — no moved-from read. Two reasons: (1) sequant::base_field(BraRange, KetRange) (index.hpp) takes its arguments by value, and bra_indices/ket_indices are passed as plain lvalues (no std::move), so they are copied, not moved. (2) The std::move(bra_indices) in the sibling arguments is only a cast to rvalue reference — the actual move-construction happens inside the delegated terminal ctor's member-init list (bra_(std::move(bra_indices))), which is sequenced strictly after all arguments of that ctor call (including base_field(...)) have been evaluated. So the field is always computed from intact indices, regardless of argument evaluation order. The comment just above the initializer notes this.

Comment thread SeQuant/core/expressions/tensor.hpp Outdated
Comment on lines +335 to +337
const Symmetry s_resolved = s.value_or(Symmetry::Nonsymm);
const ColumnSymmetry ps_resolved = ps.value_or(ColumnSymmetry::Nonsymm);
const Hermiticity h_resolved = herm_opt.value_or(Hermiticity::NonHermitian);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 81935b1. resolve_symmetries() now sources the fallbacks from Tensor::Defaults (Defaults::symmetry, Defaults::column_symmetry, Defaults::hermiticity) instead of repeating the literals, so the single source of truth actually is one.

evaleev added 3 commits July 8, 2026 17:57
…y build)

Each MBPT test TU defined its own file-local `constexpr TensorSymmetries ps`
(some in the global anonymous namespace, test_wick's inside `namespace
sequant`). In CMAKE_UNITY_BUILD=ON builds (used by the Debug/Valgrind CI
jobs) several test .cpp are concatenated into one TU, so these definitions
collided -- `reference to 'ps' is ambiguous` between the global-anonymous
and the sequant-namespace copies.

Define it once as `inline constexpr sequant::ps` in the shared
catch2_sequant.hpp (guarded, so a unity TU sees a single definition) and drop
the per-file copies. Verified with a local CMAKE_UNITY_BUILD=ON Debug build:
all 6563 assertions pass.
resolve_symmetries() documented that unspecified attributes fall back to
Tensor::Defaults but still hardcoded the literals (Symmetry::Nonsymm,
ColumnSymmetry::Nonsymm, Hermiticity::NonHermitian), so the two could
silently diverge. Use the Defaults constants directly, and clarify the
Hermiticity/BraKetSymmetry back-fill comments (addresses Copilot review).
…metric

An (anti)symmetrization operator acts on indistinguishable particles, so it
is inherently particle- (column-) symmetric, and the library already builds
its symmetrizers that way. But nothing enforced it: a symmetrizer built
programmatically without an explicit column symmetry got the (Option B)
Context-independent default of Nonsymm. When such a Nonsymm symmetrizer met
an otherwise-identical Symm one (e.g. the srcc integration test builds the
1-body S manually while the library's is Symm), the two failed to cancel,
breaking the spin-free-vs-spin-traced equality check.

Enforce column = Symm for reserved (anti)symmetrizer labels in
check_symmetries(), alongside the existing fixed-braket-symmetry rule, so a
symmetrizer is identical however it is built. Fixes the srcc/*_csv_sf
integration test; full unit suite and all srcc/stcc/eomcc/antisymmetrizer
integration variants pass locally.

@Krzmbrzl Krzmbrzl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the general design is good now

Comment thread SeQuant/core/expressions/tensor.hpp Outdated
Comment on lines +332 to +333
std::optional<Symmetry> s, std::optional<BraKetSymmetry> bks_opt,
std::optional<ColumnSymmetry> ps, std::optional<Hermiticity> herm_opt,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not take a TensorSymmetry object instead of the individual symmetry optionals?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea — done in 5bad4cd. resolve_symmetries() now takes const TensorSymmetries& (its four optionals were exactly that pack's fields), and so do the two private reserved-tag defaulting ctors. Nice side effect: the TensorSymmetries public ctors now forward the pack whole instead of unpacking it field-by-field into a positional list whose order differs from the struct's (perm, braket, column, hermiticity vs. perm, braket, hermiticity, column) — one fewer place to get that wrong.

Comment thread SeQuant/core/expressions/tensor.hpp Outdated
Comment on lines +969 to +975
// (Anti)symmetrization operators act on indistinguishable particles, so
// they are always particle- (column-) symmetric. Enforce it here -- like
// the fixed braket symmetry above -- so that a symmetrizer is identical
// however it was built (programmatically, where the column-symmetry
// default is the Context-independent Nonsymm, or by deserialization); a
// mismatch would silently prevent otherwise-equal terms from cancelling.
column_symmetry_ = ColumnSymmetry::Symm;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether it would be more consistent if we just throw an exception in this case as well (same as we do for invalid braket symmetry) 🤔

Perhaps having a make_symmetrizer and make_antisymmetrizer function similar to make_overlap would make sense (these functions would then take care of setting the symmetries exactly as needed/expected)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on both counts — done in 5bad4cd.

Factories. Added make_symmetrizer() / make_antisymmetrizer() next to make_overlap()/make_kronecker() (2- and 3-index-range overloads), built from two new named packs, symmetrizer_symmetries and antisymmetrizer_symmetries, that spell out the defining symmetries in one place. The programmatic construction sites now use them: biorthogonalization.cpp calls make_symmetrizer(), and spin.cpp::symmetrize_expr uses symmetrizer_symmetries directly (it needs a Tensor value, not an ExprPtr).

Throwing. check_symmetries() now rejects a specified column symmetry that contradicts a reserved label, exactly like the braket case, and only supplies Symm silently when the caller left it unspecified. The explicit/defaulted distinction is carried on resolved_symmetries::column_symmetry_specified, set from syms.column.has_value(). Keeping the silent fill-in for the unspecified case is deliberate: the programmatic column default is the Context-independent Nonsymm, so throwing there would make the plain Tensor(L"Ŝ", bra, ket) form unusable rather than merely unidiomatic.

Two things fell out of this that were real latent bugs, both now covered by tests:

  1. check_symmetries() applied the "(anti)symmetric bra/ket implies column symmetry" promotion after the reserved-label block, so  — which is column-symmetric by implication of its Antisymm perm symmetry alone — would have been reported as contradicting. The promotion now runs first.
  2. The deserializer forced column = Symm only for Ŝ, not Â, and it always passes a fully specified column symmetry (from the Context default or per-call options). Under a column-Nonsymm Context, deserialize(L"Â{i1,i2;a1,a2}") would therefore have handed the ctor a contradiction and thrown. It now forces Symm for both.

Also dropped a stale comment in check_symmetries() that described a Symm→Conjugate braket demotion the code has not done since the braket symmetry became a hard error.

New coverage: TEST_CASE("(anti)symmetrizer factories") in test_tensor.cpp (defining symmetries, agreement with the deserialized form, rejection of contradicting braket/column, the  promotion, and the unspecified-column fill-in) and a test_parse.cpp section that parses Ŝ/ — bare and with explicit :...-N column specs — under a column-Nonsymm Context. Full ctest: 211/211 pass.

Comment thread tests/unit/catch2_sequant.hpp Outdated
/// Defined `inline` in this shared header (rather than once per TU) so that
/// unity/jumbo test builds see a single definition instead of colliding
/// anonymous-namespace copies.
inline constexpr TensorSymmetries ps{.column = ColumnSymmetry::Symm};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for a more descriptive name. Just having ps, appear in the test cases seems confusing to someone who doesn't already know what this is. Maybe something like def_symms?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, renamed in afad235 — I went with particle_symmetric rather than def_symms. Both are more descriptive than ps, but particle_symmetric says what the pack is (column = Symm), whereas def_symms only says it is some bundle of defaults, which at a call site like Tensor(L"g", bra{…}, ket{…}, def_symms) still leaves you looking up the definition. It also matches the example already spelled out in the TensorSymmetries docs. Happy to switch if you prefer def_symms. Renamed the thc.cpp copy too, for consistency.

Comment thread tests/unit/test_main.cpp
Comment on lines +52 to +56
// mbpt works with particle-symmetric tensors, so default new/parsed
// tensors to particle (column) symmetry; bra-ket and permutational
// symmetries stay at the safe Nonsymm/NonHermitian library defaults and
// are specified explicitly where needed (e.g. Hermitian integrals)
.column_symmetry = ColumnSymmetry::Symm});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally our test cases should not use non-default settings throughout. I think it would be cleaner if we modified the actual input strings to include the symmetries explicitly instead 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep this one as is, but let me lay out the reasoning rather than just assert it.

The harness is an MBPT application, not a neutral one. test_main.cpp already sets seven non-default Context knobs — vacuum = SingleProduct, metric = Unit, spbasis = Spinor, first_dummy_index_ordinal = 100, the canonicalization method, and both typesetting knobs — precisely because the suite exercises the MBPT domain. column_symmetry = Symm is the same kind of knob, and the two integration apps (srcc, stcc) set it identically, so the unit tests are configured the way a real client configures SeQuant. Making the harness diverge from that would test a configuration nothing actually uses.

Cost. I measured it: dropping the setting breaks 12 of 50 test cases, and restoring them by spelling out symmetries in the inputs means annotating on the order of 1750 tensor literals across seven files — 556 in test_spin.cpp, 454 in the sf_r2_direct_real_inc.hpp reference fixture, 244 in test_tensor_network.cpp, and so on. Since many of those literals are expected serialization outputs, not just inputs, they change too. That is a large mechanical diff whose main effect is to bury what each test is actually asserting.

The mechanism is already tested without relying on the global. test_parse.cpp has SECTION("default symmetries follow the Context"), which flips the Context column default both ways under set_scoped_default_context and checks the deserialized tensor each time, and test_tensor.cpp has SECTION("programmatic ctor defaults are Context-independent"), which sets all three Context defaults to non-default values and confirms the ctors ignore them. This PR adds two more scoped-Context sections (Hermitian default, column-Nonsymm default). So the global setting is not masking behavior — anything that depends on it is pinned explicitly somewhere.

If you still want the harness on library defaults, I'm happy to do it, but I'd suggest a separate PR: it is ~1750 mechanical edits that would otherwise dominate the diff of this one, and it's a change to test conventions rather than to the feature under review.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is a large change that creates a very noise and quite uninformative diff. So a separate PR might indeed be best.

test_main.cpp already sets seven non-default Context knobs — vacuum = SingleProduct, metric = Unit, spbasis = Spinor, first_dummy_index_ordinal = 100, the canonicalization method, and both typesetting knobs

The currently set knobs have mostly minor effects in where and how they change processing. Symmetry, however, is a very central quantity and therefore I think it should be considered to be more important than the other settings. And while it is true that most tests are MBPT specifc, I think it is very important that these functions do the correct thing with the default settings as that is what I expect the vast majority of users to use.

@evaleev evaleev added this to the 2.3 milestone Jul 27, 2026
evaleev added 3 commits August 4, 2026 12:59
…er factories

Addresses two review comments (Krzmbrzl):

1. `resolve_symmetries()` took four parallel `std::optional` symmetry
   arguments that are exactly the fields of the existing
   TensorSymmetries named-parameter pack -- take the pack instead. The
   private reserved-tag defaulting ctors follow suit, which also lets
   the TensorSymmetries public ctors forward the pack whole instead of
   unpacking it field-by-field.

2. Reserved (anti)symmetrizers silently had their column symmetry
   forced to Symm; be consistent with the braket-symmetry handling and
   *throw* when a contradicting column symmetry was spelled out, while
   still supplying the correct one when it was merely left unspecified
   (so the Context-independent Nonsymm column default cannot produce a
   symmetrizer that differs from a deserialized one). Add
   make_symmetrizer()/make_antisymmetrizer() (cf. make_overlap) plus
   the `symmetrizer_symmetries`/`antisymmetrizer_symmetries` packs they
   are built from, and use them at the mbpt construction sites.

Two supporting fixes fall out of (2):
- check_symmetries() now applies the "(anti)symmetric bra/ket implies
  column symmetry" promotion *first*, so  -- column-symmetric by
  implication of its Antisymm perm symmetry alone -- is never reported
  as contradicting.
- the deserializer forces column = Symm for  as well as Ŝ; it always
  passes a fully specified column symmetry, so under a column-Nonsymm
  Context it would otherwise hand the ctor a contradiction.

Also drops a stale comment in check_symmetries() describing a
Symm -> Conjugate braket demotion that the code has not done since the
braket symmetry became an error.
Review feedback (Krzmbrzl): a bare `ps` at ~180 construction sites is
opaque to anyone who hasn't read its definition. `particle_symmetric`
says what the pack *is* (column = Symm) rather than merely that it is a
default, and matches the example spelled out in the TensorSymmetries
docs. Renamed in the thc.cpp copy too, for consistency.
The two delta factors in `F()` were built through the public Tensor
ctor, so their column symmetry fell back to the Context-independent
`Nonsymm` default, while `make_kronecker()` (and the deserializer)
build them column-`Symm`. Column symmetry participates in the tensor
hash, so the two spellings of the same delta compare unequal and
otherwise-identical terms would not merge -- exactly the failure mode
`make_overlap`/`make_kronecker` carry a comment about. The adjacent `g`
factors in the same expression already had the explicit `Symm`; the
deltas were missed.
@evaleev

evaleev commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

Addressed the outstanding review comments (@Krzmbrzl); pushed as 5bad4cd, afad235, 4b9d511.

  • resolve_symmetries() takes a TensorSymmetries instead of four parallel optionals, and so do the private reserved-tag ctors.
  • make_symmetrizer() / make_antisymmetrizer() added alongside make_overlap(), built from new symmetrizer_symmetries / antisymmetrizer_symmetries packs, and used at the programmatic construction sites. check_symmetries() now throws on a contradicting explicit column symmetry (consistent with the braket case), and only fills one in silently when it was left unspecified.
  • ps renamed to particle_symmetric in the test TUs and thc.cpp.
  • Kept test_main.cpp's Context column default, with reasoning in the thread — happy to revisit as a follow-up.

Two latent bugs surfaced while making the ctor stricter, both now covered by tests:

  1. check_symmetries() ran the "(anti)symmetric bra/ket implies column symmetry" promotion after the reserved-label check, so  would have been misreported as contradicting. Reordered.
  2. The deserializer forced column = Symm for Ŝ but not Â, while always passing a fully specified column symmetry — so under a column-Nonsymm Context, parsing Â{…} would have thrown. Now forced for both.

And one pre-existing inconsistency, fixed in 4b9d511: the two Kronecker deltas in mbpt::F() were built through the public Tensor ctor, so they came out column-Nonsymm while make_kronecker() and the deserializer produce column-Symm. Column symmetry participates in the tensor hash, so the two spellings of the same δ compare unequal and otherwise-identical terms would not merge. The adjacent g factors in the same expression already carried the explicit Symm; the deltas were missed.

Verification: full ctest is 211/211 (unit, srcc/stcc integration, doc examples, external-interface), and all changed files pass clang-format-17 --Werror.

@evaleev

evaleev commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

@Krzmbrzl have a look. All comments are from claude.

@Krzmbrzl Krzmbrzl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like Claude added a bunch of unnecessary comments that I personally would remove (as they don't really help understanding the code) but that's certainly not a blocker.

Comment thread SeQuant/core/expressions/tensor.hpp Outdated

/// the four symmetry attributes of a Tensor, fully resolved (no defaulting
/// left to do)
struct resolved_symmetries {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
struct resolved_symmetries {
struct ResolvedSymmetries {

to keep naming consistent

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed in edf0140.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

SeQuant/core/context.hpp:69

  • This new paragraph says Context has no bra/ket symmetry knob, but the class-level documentation above still lists braket_symmetry as Context state at context.hpp:25-31, even though that member was removed. The contradictory public documentation can lead callers to look for a nonexistent setting; please remove or rewrite the stale class-level entry.
    // default symmetries used when *deserializing* a tensor whose symmetry is
    // under-specified in the input; the library defaults are the *safest* (most
    // general) possible, and applications can fine-tune them via Context for
    // ergonomics (e.g. mbpt assumes particle-symmetric tensors). These do NOT
    // affect the programmatic Tensor ctors, whose defaults are fixed and
    // independent of the ambient Context. Note that there is no braket-symmetry
    // default: braket symmetry is a *derived* property of a tensor (from its
    // #Hermiticity and #base_field), so #hermiticity is the knob instead (cf.
    // removal of Context::braket_symmetry).

Comment on lines +342 to +346
const Symmetry s_resolved = syms.perm.value_or(Defaults::symmetry);
const ColumnSymmetry ps_resolved =
syms.column.value_or(Defaults::column_symmetry);
const Hermiticity h_resolved =
syms.hermiticity.value_or(Defaults::hermiticity);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is by design, not an oversight — but the PR description was stale and is what says otherwise, so the confusion is fair.

The Context-driven defaulting in the original version of this PR did apply to the programmatic ctors. That was reverted in a7f9dea ("make programmatic symmetry defaults Context-free"): the meaning of a Tensor ctor call should not depend on ambient global state, and the hot construction path should not read a shared, mutable global. So resolve_symmetries() falls back to Tensor::Defaults only, and the Context symmetry defaults are honored at exactly one place — the deserializer (to_default_symms()), which is the boundary that turns under-specified external input into tensors.

Applications that want a different programmatic default spell it out; the TensorSymmetries pack added here exists precisely so that can be a one-liner per TU rather than an argument at every call site (see thc.cpp, rdm.cpp, catch2_sequant.hpp). This is pinned by test_tensor.cpp's SECTION("programmatic ctor defaults are Context-independent"), which sets all three Context defaults to non-default values and asserts the ctors ignore them.

The PR body still described the pre-a7f9dea3d behavior; I've corrected that bullet.

/// derived from this and the tensor's base field
static constexpr Hermiticity hermiticity = Hermiticity::NonHermitian;
/// default particle-exchange ColumnSymmetry
static constexpr ColumnSymmetry column_symmetry = ColumnSymmetry::Nonsymm;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real bug — fixed in 5f7c3c9.

Confirmed the attribution: on the base branch (#553) the programmatic column default was still ColumnSymmetry::Symm, so it is a7f9dea in this PR that flipped rdm.cpp's γ/κ from column-Symm to column-Nonsymm. Meanwhile the parallel RDM path in op.cpp was updated to request Symm explicitly, so the same γ built two ways would no longer compare equal — column symmetry participates in the tensor hash, so otherwise-identical terms stop merging. Exactly the class of bug as the F() Kronecker deltas fixed in 4b9d511; rdm.cpp was simply missed in the same sweep.

Fixed the way thc.cpp does it: a TU-local constexpr TensorSymmetries particle_symmetric{.column = ColumnSymmetry::Symm} fed at all 21 construction sites (γ, κ, and the DE2/DDE/D2E intermediates built from them — all over the same indistinguishable particles), which also restores the pre-PR symmetry for every one of them rather than only the two you named.

ctest green, including sequant/integration/antisymmetrizer, the one test that actually drives decompositions::three_body_substitution.

Comment on lines 334 to +336
if (tensor.name == reserved::antisymm_label()) {
perm_symm = Symmetry::Antisymm;
} else if (tensor.name == reserved::symm_label()) {
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also real — fixed in 5f7c3c9.

Verified before fixing: under ctx.set(Symmetry::Antisymm), deserialize<ExprPtr>(L"Ŝ{i1,i2;a1,a2}") came back with symmetry() == Antisymm, so it did not compare equal to make_symmetrizer() (which is .perm = Nonsymm). check_symmetries() does not repair it, since Ŝ's perm symmetry has no reserved-label rule the way its braket and column symmetries do.

Went with forcing Symmetry::Nonsymm, mirroring how Â's Antisymm is already forced unconditionally rather than being validated-and-rejected — the two labels now get symmetric treatment:

if (tensor.name == reserved::antisymm_label()) {
  perm_symm = Symmetry::Antisymm;
} else if (tensor.name == reserved::symm_label()) {
  perm_symm = Symmetry::Nonsymm;
}

New SECTION("(anti)symmetrization operators have fixed perm symmetry") in test_parse.cpp parses both operators under a Symmetry::Antisymm Context and asserts equality with make_symmetrizer() / make_antisymmetrizer(). It fails on the previous commit and passes now.

That closes the third and last of the three defining symmetries: braket (7fdb794), column (47869fb), and now perm.

Addresses the two follow-ups on #557:

- rename the private `resolved_symmetries` helper struct to
  `ResolvedSymmetries`, for consistency with `TensorSymmetries`
- trim the commentary added by this PR down to what is not already
  evident from the code: comments that merely restated the call being
  made (`request Symm explicitly` because `ColumnSymmetry::Symm` is
  right there, `unspecified attributes fall back to ...` above a
  `value_or(Defaults::...)`) are gone; the domain facts (mbpt tensors
  act on indistinguishable particles) and the non-obvious invariants
  (ordering constraint in check_symmetries, why the deserializer forces
  the reserved operators' symmetries, why base_field() reads intact
  indices) are kept, condensed.
1. `deserialize` left the reserved symmetrizer Ŝ's bra/ket permutational
   symmetry to be inherited from the Context, so under a Context with
   `symmetry = (Anti)symm` a bare `Ŝ{...}` parsed as perm-(Anti)symm and
   did not compare equal to `make_symmetrizer()`. Ŝ is perm-Nonsymm by
   definition (it symmetrizes the {bra,ket} particle columns, not within
   bra/ket), so force it, exactly as Â's Antisymm is already forced.
   Regression test added in test_parse.cpp.

2. `mbpt::decompositions` (rdm.cpp) built its γ/κ RDMs and cumulants
   through the plain Tensor ctor, so making the programmatic column
   default `Nonsymm` (a7f9dea) silently turned them column-Nonsymm --
   while the parallel RDM path in `op.cpp` requests `Symm`. Column
   symmetry participates in the tensor hash, so the two spellings of the
   same γ compare unequal and otherwise-identical terms stop merging.
   Same class of bug as the F() Kronecker deltas fixed in 4b9d511.
   Fixed with a TU-local `particle_symmetric` pack, as in thc.cpp.

Also refreshed the stale `Context` class-level docs, which still listed
the `braket_symmetry` member removed in 8c4669d and did not mention
the symmetry defaults this PR adds.
@evaleev
evaleev merged commit 98e3727 into default-tensor-symmetry Aug 23, 2026
16 checks passed
@evaleev
evaleev deleted the default-tensor-symmetry-context branch August 23, 2026 23:17
@evaleev

evaleev commented Aug 23, 2026

Copy link
Copy Markdown
Member Author

Pushed edf0140 and 5f7c3c9.

@Krzmbrzl's two follow-ups.

  • resolved_symmetriesResolvedSymmetries, per your suggestion.
  • Comment cleanup (your note on the approval): agreed, and trimmed — net −68 comment lines across 9 files. The rule I applied was does the comment say something the code does not. Gone: everything that restated the call being made (// ... so request Symm explicitly sitting directly above ColumnSymmetry::Symm, // unspecified attributes fall back to the single source of truth above a value_or(Defaults::...), the four near-identical three-line repeats of the same mbpt rationale in op.cpp). Kept, condensed: the domain facts that are genuinely not in the code (mbpt tensors act on indistinguishable particles) and the non-obvious invariants — the ordering constraint in check_symmetries(), why the deserializer must force rather than pass through the reserved operators' symmetries, and why base_field() reads intact indices in the move-form delegation. Happy to cut further if any of those still read as noise to you.

Copilot review. Two of the three findings were real and are fixed; I pushed back on the third.

  1. Parsed Ŝ inherited the Context's perm symmetry. Under a Symmetry::Antisymm Context, a bare Ŝ{i1,i2;a1,a2} parsed as perm-Antisymm and so did not compare equal to make_symmetrizer(). Ŝ is perm-Nonsymm by definition (it symmetrizes the {bra,ket} particle columns, not within bra/ket), so it is now forced, mirroring Â's Antisymm. This closes the third and last of the three defining symmetries — braket (7fdb794), column (47869fb), perm (now). New test_parse.cpp section, which fails on the previous commit.

  2. mbpt::decompositions (rdm.cpp) was missed by the column-default flip. Checked the attribution: the base branch (Make tensors non-symmetric by default #553) still defaulted the programmatic column symmetry to Symm, so it is a7f9dea in this PR that turned rdm.cpp's γ/κ column-Nonsymm — while the parallel RDM path in op.cpp was updated to request Symm. Column symmetry participates in the tensor hash, so the same γ built the two ways compares unequal and otherwise-identical terms stop merging. Same bug as the F() Kronecker deltas fixed in 4b9d511; rdm.cpp was simply missed in that sweep. Fixed with a TU-local particle_symmetric pack as in thc.cpp, applied at all 21 sites.

  3. "The ctors bypass the Context defaults." By design — that is a7f9dea (Option B): programmatic construction is Context-independent, and the deserializer is the only place that honors the Context symmetry defaults. What misled the review is that the PR description still described the pre-a7f9dea3d behavior; I've corrected that bullet. The behavior is pinned by test_tensor.cpp's SECTION("programmatic ctor defaults are Context-independent").

Also refreshed the stale Context class-level docs, which still listed the braket_symmetry member removed in 8c4669d (Copilot flagged this as a low-confidence note) and did not mention the three defaults this PR adds.

Verification. Debug build, clang, all changed files clean under clang-format-17 --Werror. check-sequant 71/71, and all 9 sequant/integration/* tests pass — including sequant/integration/antisymmetrizer, the one test that actually drives decompositions::three_body_substitution. (Fewer cases than the 211 I quoted earlier: this build has the TiledArray/BTAS/TAPP/Torch backends and the Python bindings off.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants