Azam/feature/hardcoded biorthogonal coefficients #457

ABesharat · 2025-12-25T20:26:42Z

This branch provides hardcoded biorthgonal and nns coefficients

…nal_coefficients' into azam/feature/hardcoded_biorthogonal_coefficients

…eature/hardcoded_biorthogonal_coefficients # Conflicts: # SeQuant/domain/mbpt/biorthogonalization.cpp

…eature/hardcoded_biorthogonal_coefficients

…se use computed ones

…discarded terms

…eature/hardcoded_biorthogonal_coefficients

…coded(rational) and computed(double) coeffs

…ded_biorthogonal_coefficients' into azam/feature/hardcoded_biorthogonal_coefficients

…eature/hardcoded_biorthogonal_coefficients

… header file

TODO fix test_eval_* tests

Copilot

Pull request overview

This PR introduces hardcoded (exact) biorthogonalization coefficients and NNS projection weights to avoid numerical precision loss, and updates evaluation/tests to use the new NNS projection path.

Changes:

Add hardcoded biorthogonal coefficient generation (ranks 1–5) with memoization fallback to computed coefficients for higher ranks.
Introduce NNS projection weights (hardcoded up to rank 5, computed beyond) and TA/BTAS projection helpers.
Update spintracing normalization behavior and adjust evaluation unit tests accordingly.

Reviewed changes

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

Show a summary per file

File	Description
`SeQuant/domain/mbpt/biorthogonalization.hpp`	Declares hardcoded coefficient APIs, adds NNS projector weights + TA/BTAS projection helpers.
`SeQuant/domain/mbpt/biorthogonalization.cpp`	Implements hardcoded coefficient matrices, memoized coefficient selection, and computed NNS weights.
`SeQuant/domain/mbpt/spin.cpp`	Simplifies S normalization factor handling in `closed_shell_CC_spintrace_v2`.
`SeQuant/core/eval/result.hpp`	Removes Result-level NNS projection implementation (migrated out).
`SeQuant/core/eval/eval.hpp`	Removes `EvalMode::BiorthogonalNNSProject` and `evaluate_biorthogonal_nns_project`.
`tests/unit/test_eval_ta.cpp`	Updates TA evaluation to call the new NNS projection helper and adds rank-4 NNS test.
`tests/unit/test_eval_btas.cpp`	Updates BTAS evaluation to call the new NNS projection helper and adjusts permutation averaging.
`tests/unit/test_spin.cpp`	Updates comments/section naming reflecting new spintracing behavior.
`CMakeLists.txt`	Adds compile definitions for evaluation-related feature macros.

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

Copilot · 2026-01-23T03:12:41Z

tests/unit/test_eval_ta.cpp

      REQUIRE(norm(man2) == Catch::Approx(norm(eval2)));
      TArrayD zero2;
      zero2("0,1,2,3,4,5") = man2("0,1,2,3,4,5") - eval2("0,1,2,3,4,5");
      REQUIRE(norm(zero1) == Catch::Approx(0).margin(


This assertion is checking norm(zero1) again after computing zero2. It should validate zero2 (the rank-3 residual case), otherwise the test can pass even if eval2 is wrong.

Suggested change

REQUIRE(norm(zero1) == Catch::Approx(0).margin(

REQUIRE(norm(zero2) == Catch::Approx(0).margin(

Copilot · 2026-01-23T03:12:42Z

SeQuant/domain/mbpt/biorthogonalization.hpp

+/// \return Vector of NNS projector weights representing the last row
+///
+/// \throw std::runtime_error if n_particles is not in the range [1,5]


The comment for hardcoded_nns_projector claims it throws std::runtime_error when n_particles is out of range, but the function actually returns std::nullopt. Update the documentation (or change behavior) so callers know what to expect.

Suggested change

/// \return Vector of NNS projector weights representing the last row

///

/// \throw std::runtime_error if n_particles is not in the range [1,5]

/// \return Optional vector of NNS projector weights representing the last row;

/// returns std::nullopt if n_particles is not in the supported range.

Copilot · 2026-01-23T03:12:42Z

SeQuant/domain/mbpt/biorthogonalization.cpp

+  auto memoize = []<typename T>(container::map<std::pair<std::size_t, double>,
+                                               std::optional<T>>& cache,
+                                std::mutex& mutex, std::condition_variable& cv,
+                                std::pair<std::size_t, double> key,
+                                auto compute_fn) -> const T& {
+    {
+      std::unique_lock<std::mutex> lock(mutex);
+      auto [it, inserted] = cache.try_emplace(key, std::nullopt);
+      if (!inserted) {
+        cv.wait(lock, [&] { return it->second.has_value(); });
+        return it->second.value();
+      }
+    }
+
+    T result = compute_fn();
+
+    {
+      std::lock_guard<std::mutex> lock(mutex);
+      cache[key] = std::move(result);
+      cv.notify_all();
+      return cache[key].value();
+    }


The local memoize helper can deadlock if compute_fn throws: the inserted cache entry stays std::nullopt and any concurrent callers will wait forever on the condition_variable. Add exception handling to ensure the cache is updated/erased and cv.notify_all() is called on failure.

Copilot · 2026-01-23T03:12:42Z

CMakeLists.txt

 endif()
 if (TARGET tiledarray)
    target_link_libraries(SeQuant PUBLIC tiledarray)
+    target_compile_definitions(SeQuant PUBLIC SEQUANT_HAS_TILEDARRAY=1 SEQUANT_HAS_BTAS=1)


This sets SEQUANT_HAS_BTAS=1 whenever TARGET tiledarray exists. If BTAS availability is independent of TiledArray, this can incorrectly enable BTAS-dependent code paths/headers. Consider defining SEQUANT_HAS_BTAS only when BTAS is actually found/available (or drop it if BTAS is always required).

Suggested change

target_compile_definitions(SeQuant PUBLIC SEQUANT_HAS_TILEDARRAY=1 SEQUANT_HAS_BTAS=1)

target_compile_definitions(SeQuant PUBLIC SEQUANT_HAS_TILEDARRAY=1)

Copilot · 2026-01-23T03:12:43Z

SeQuant/domain/mbpt/biorthogonalization.hpp

+/// The NNS projector weights are obtained from the normalized pseudoinverse
+/// of M: first compute M_pinv (the pseudoinverse), then normalize it by the
+/// factor ((n_particles)!/rank(M)).
+//// Finally, NNS projector = normalized_M_pinv · M.


There’s an extra slash in the doc comment (//// Finally, ...). Consider changing it to a normal /// line so it renders correctly in generated docs.

Suggested change

//// Finally, NNS projector = normalized_M_pinv · M.

/// Finally, NNS projector = normalized_M_pinv · M.

Copilot · 2026-01-23T03:12:43Z

SeQuant/domain/mbpt/biorthogonalization.hpp

+#include <SeQuant/core/eval/eval.hpp>
+#endif
+
+#include <condition_variable>


This header now uses std::optional, std::vector, std::mutex, and std::floating_point, but it only includes <condition_variable>. Add the required standard headers (at least , , , , and /) so the header is self-contained and doesn’t rely on transitive includes.

Suggested change

#include <condition_variable>

#include <condition_variable>

#include <optional>

#include <vector>

#include <mutex>

#include <concepts>

#include <cstddef>

Copilot · 2026-01-23T03:12:43Z

SeQuant/domain/mbpt/biorthogonalization.hpp

 /// performs symbolic biorthogonal transform of CC-like equation using
 ///(for rank-3 and higher
 /// Wang-Knizia biorthogonalization (https://arxiv.org/abs/1805.00565) is used
+/// uses hardcoded coefficients for ranks 1-6, computed coefficients for higher


Docstring says biorthogonal_transform uses hardcoded coefficients for ranks 1-6, but the implementation hardcodes only up to rank 5 (max_rank_hardcoded_biorth_coeffs = 5, and hardcoded_biorth_coeffs_first_row throws for >5). Please update the comment to match the actual supported range.

Suggested change

/// uses hardcoded coefficients for ranks 1-6, computed coefficients for higher

/// uses hardcoded coefficients for ranks 1-5, computed coefficients for higher

Copilot · 2026-01-23T03:12:43Z

SeQuant/domain/mbpt/biorthogonalization.hpp

+  static std::mutex cache_mutex;
+  static std::condition_variable cache_cv;
+  static container::map<CacheKey, CacheValue> cache;
+
+  CacheKey key{n_particles, threshold};
+
+  {
+    std::unique_lock<std::mutex> lock(cache_mutex);
+    auto [it, inserted] = cache.try_emplace(key, std::nullopt);
+    if (!inserted) {
+      cache_cv.wait(lock, [&] { return it->second.has_value(); });
+      return it->second.value();
+    }
+  }


nns_projection_weights memoization can deadlock: if computing coefficients throws, the cache entry remains std::nullopt and any waiting threads will block forever. Consider storing an exception state (e.g., std::exception_ptr) or erasing the key and notifying in a catch block so waiters don’t hang.

Copilot · 2026-01-23T03:12:44Z

SeQuant/domain/mbpt/spin.cpp

+    const auto nf = ex<Constant>(
+        rational{1, factorial(ext_idxs.size())});  // normalization factor for S


This change removes the previous (fact_n/(fact_n-1)) rescaling for ext rank > 2 and always uses 1/factorial(ext_idxs.size()). There is still code elsewhere applying the old rescaling (e.g., tests/integration/srcc.cpp:200-210). Please align those call sites/tests or centralize the normalization so spintrace-v2 and other spin-free generation paths stay consistent.

Suggested change

const auto nf = ex<Constant>(

rational{1, factorial(ext_idxs.size())}); // normalization factor for S

const auto n_ext = ext_idxs.size();

const auto nf = ex<Constant>(

rational{1, factorial(n_ext > 2 ? n_ext - 1 : n_ext)}); // normalization factor for S

ABesharat added 30 commits October 13, 2025 04:35

set hardcoded biorthogonal coeffs till rank 7

e1dba87

rank 7

1323fc6

discard redundant assertion

897a42a

better name: computed_coefficients

a22809c

add hardcoded file to cmakelist

dbcd4e5

v2: correct rescaling factor

8fd5be0

renaming original coeffs to computed_coefficients

47ccfe1

Merge remote-tracking branch 'origin/azam/feature/hardcoded_biorthogo…

93829df

…nal_coefficients' into azam/feature/hardcoded_biorthogonal_coefficients

renaming

70ef68a

Merge remote-tracking branch 'refs/remotes/origin/master' into azam/f…

4fc0898

…eature/hardcoded_biorthogonal_coefficients # Conflicts: # SeQuant/domain/mbpt/biorthogonalization.cpp

discard old coefficients

fc0dcda

discard only the old coefficients, not num_perms

f309f62

no inline anymore for performance

b5da4b8

Merge remote-tracking branch 'refs/remotes/origin/master' into azam/f…

25bb6f4

…eature/hardcoded_biorthogonal_coefficients

at max rank 6 is enough

3926bde

Merge remote-tracking branch 'refs/remotes/origin/master' into azam/f…

e485b9c

…eature/hardcoded_biorthogonal_coefficients

test_eval_ta: add rank 4 nns test, fix rank 3

87b8177

test_eval_btas: fix nns for rank 3

5ca6d55

add hardcoded biortho coeffs and nns projection

188825c

fix include

8e66ac7

use hardcoded biortho coeffs for rank<=6, otherwise use computed ones

1dc89b3

do not call nns with int type ,use hardcoded nns for rank<=6, otherwi…

53b47be

…se use computed ones

make libperm public to build the same order of permutations

a28d886

fix include for Eigen!

ab8f453

change the name of hardcoded nns fn

d5b20f6

renaming again!

278dcc3

renaming hardcoded_biortho_coeffs fn

aede8b3

change assert to SEQUANT_ASSERT

4b1a92f

no need to have rescaling for v2. new nns is enough for restoring WK …

8ee5a57

…discarded terms

documentation correction

a3345c4

evaleev approved these changes Jan 7, 2026

View reviewed changes

ABesharat and others added 20 commits January 11, 2026 12:49

add some documentations

aa7a41c

clear documentation

69ab3a1

Merge remote-tracking branch 'refs/remotes/origin/master' into azam/f…

af93394

…eature/hardcoded_biorthogonal_coefficients

apply encapsulation for nns

2d10389

memoize nns_projection weights

a9a17b1

add include

afed9c4

dox cleanup

1ca7fa6

nns_projection_weights: no third lock needed to return the result

ff05b7c

removed the no-longer-used biorthogonalization_hardcoded.ipp

4732bf5

memoize biorthogonal coeffs by lambda to have separate cache for hard…

ecde7eb

…coded(rational) and computed(double) coeffs

Merge remote-tracking branch 'refs/remotes/origin/azam/feature/hardco…

5f490ef

…ded_biorthogonal_coefficients' into azam/feature/hardcoded_biorthogonal_coefficients

Merge remote-tracking branch 'refs/remotes/origin/master' into azam/f…

333f296

…eature/hardcoded_biorthogonal_coefficients

Merge remote-tracking branch 'refs/remotes/origin/master' into azam/f…

6697a4e

…eature/hardcoded_biorthogonal_coefficients

remove the redundant header include

8f42dac

move Eigen usage from public header to implementation

2e171a8

remove the redundant header include

370bd30

biorthogonalization.cpp: forward declaration to avoid having Eigen in…

e6adbab

… header file

move hardcoded contents to biorthogonalization.cpp and hpp

72dac7e

add to header file to keep the documentation

711e460

make compute-nns compact

f8f3115

ABesharat force-pushed the azam/feature/hardcoded_biorthogonal_coefficients branch from ae2bb57 to f8f3115 Compare January 18, 2026 23:31

ABesharat and others added 4 commits January 20, 2026 23:21

using auto instead of std::size_t

9d98421

remove biorthogonalization from IR since it is too domain-specific

c3a6c07

TODO fix test_eval_* tests

fix the cmake

c8cf5b9

fix nns call in eval_ta and eval_btas

6c138ef

evaleev approved these changes Jan 23, 2026

View reviewed changes

evaleev requested a review from Copilot January 23, 2026 03:06

Copilot started reviewing on behalf of evaleev January 23, 2026 03:06 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azam/feature/hardcoded biorthogonal coefficients #457

Azam/feature/hardcoded biorthogonal coefficients #457

ABesharat commented Dec 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	REQUIRE(norm(zero1) == Catch::Approx(0).margin(
	REQUIRE(norm(zero2) == Catch::Approx(0).margin(

	target_compile_definitions(SeQuant PUBLIC SEQUANT_HAS_TILEDARRAY=1 SEQUANT_HAS_BTAS=1)
	target_compile_definitions(SeQuant PUBLIC SEQUANT_HAS_TILEDARRAY=1)

	//// Finally, NNS projector = normalized_M_pinv · M.
	/// Finally, NNS projector = normalized_M_pinv · M.

-#include <condition_variable>
+#include <condition_variable>
+#include <optional>
+#include <vector>
+#include <mutex>
+#include <concepts>
+#include <cstddef>

	/// uses hardcoded coefficients for ranks 1-6, computed coefficients for higher
	/// uses hardcoded coefficients for ranks 1-5, computed coefficients for higher

		const auto nf = ex<Constant>(
		rational{1, factorial(ext_idxs.size())}); // normalization factor for S

Azam/feature/hardcoded biorthogonal coefficients #457

Are you sure you want to change the base?

Azam/feature/hardcoded biorthogonal coefficients #457

Conversation

ABesharat commented Dec 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants