Make external index handling more robust#476
Make external index handling more robust#476Krzmbrzl wants to merge 11 commits intoValeevGroup:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the robustness of external index handling by introducing explicit slot tracking instead of relying on ordering conventions. The change replaces implicit positional semantics with an explicit SlottedIndex type that combines an Index with a Slot enum (Bra, Ket, or Aux).
Changes:
- Introduced
SlottedIndexclass andSlotenum to explicitly track index positions - Updated
external_indices()and related utility functions to returnSlottedIndexgroups - Modified all spin-tracing and biorthogonalization APIs to accept
SlottedIndexcontainers
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| SeQuant/core/slotted_index.hpp | New class combining Index with Slot information |
| SeQuant/core/attr.hpp | Added Slot enum for Bra/Ket/Aux identification |
| SeQuant/core/meta.hpp | Added type traits for const-correctness handling |
| SeQuant/core/utility/indices.hpp | Updated external_indices, get_bra_idx, get_ket_idx to use SlottedIndex; added concepts |
| SeQuant/core/expressions/result_expr.hpp | Updated index_particle_grouping to construct SlottedIndex groups |
| SeQuant/domain/mbpt/spin.hpp | Updated function signatures to accept SlottedIndex containers |
| SeQuant/domain/mbpt/spin.cpp | Implementation changes to work with SlottedIndex semantics |
| SeQuant/domain/mbpt/biorthogonalization.hpp/cpp | Updated signatures for SlottedIndex |
| SeQuant/domain/mbpt/op.hpp | Added [[maybe_unused]] attribute to suppress warning |
| tests/unit/test_wick.cpp | Updated test calls to use SlottedIndex with explicit slots |
| tests/unit/test_utilities.cpp | Updated tests with SlottedIndex expected values |
| tests/unit/test_spin.cpp | Updated all spintrace calls with SlottedIndex syntax |
| tests/integration/srcc.cpp | Updated to use get_bra_idx/get_ket_idx helpers |
| CMakeLists.txt | Added slotted_index.hpp to build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ExprPtr closed_shell_spintrace( | ||
| const ExprPtr& expression, | ||
| const container::svector<container::svector<Index>>& ext_index_groups, | ||
| const container::svector<container::svector<SlottedIndex>>& |
There was a problem hiding this comment.
I was envisioning that index group specifications would not need slot into, to make it easier to specify. Besides, each index can live in multiple slots ...
There was a problem hiding this comment.
To my understanding external indices can't live in multiple slots, can they?
And since we do seem to selectively access bra and ket indices inside at least some of the spintracing functions, I figured that passing SlottedIndex is more explicit. Plus, this makes using external_indices() more straight forward as we don't have to convert down to plain Index types.
There was a problem hiding this comment.
Re: making it easier to specify
I was thinking about adding an overload that would work on plain Index object and determines the slot type based on the currently used implicit ordering. This could then simply forward to the new spintrace overload. However, I ended up not doing that because
- This would re-introduce the issue of depending on implicit ordering conventions which are easy to mes up (which is the whole point of this PR in the first place)
- We'd have to make a copy of the containers representing the external indices, unless we wanted to make the entire spintrace function a template, which could work on (projected) views. I prefer having as little function definitions in headers as possible though - particularly large ones like spintracing.
There was a problem hiding this comment.
external indices can live in multiple slots. E.g. Khatri-Rao product: A{i,j} B{i,k} -> C{i,j,k}. i appears in both A and B and also in the result.
Now, such indices cannot be deduced from TN itself. So deduction of external indices is OK to produce {Index,Slot} pairs as this is the only possible outcome (strictly speaking this is a simplification for now, as PNO-like indices introduce slots on indices, and same index can appear in both tensor and index slots ... not worth considering yet). But specifying when we receive external indices from the user, we should not assume that each index has a single slot. The user expects to be able to say "here are indices, I am too lazy to look this up, yes me looking it up would provide extra security, but most operations do not require knowledge of where the external indices sit".
So my proposal is that the external index groups passed as arguments to functions like closed_shell_spintrace can accept both container::svector<container::svector<SlottedIndex>> and container::svector<container::svector<Index>>, the former can be lowered internally into the latter (either by explicit conversion, or by making a view of indices).
613d17b to
907981a
Compare
We now explicitly track the slot (kind) of a given external index instead of relying on ordering conventions to encode this information.
907981a to
cf2ecf7
Compare
We now explicitly track the slot (kind) of a given external index
instead of relying on ordering conventions to encode this information.