Skip to content

Handle full contraction in Flops computation#474

Merged
bimalgaudel merged 3 commits intomasterfrom
ajay/fix/asy-cost-scalar-result
Feb 2, 2026
Merged

Handle full contraction in Flops computation#474
bimalgaudel merged 3 commits intomasterfrom
ajay/fix/asy-cost-scalar-result

Conversation

@ajay-mk
Copy link
Member

@ajay-mk ajay-mk commented Feb 2, 2026

  • Update Flops::operator() to handle the case where tensor × tensor → scalar
  • Test with CC energy-like expression

Previously Flops::operator() assumed tensor products produced a tensor as a result. This throws an error from ContractedIndexCount when all indices are contracted.
Full contraction is now handled correctly and cost is computed  as 2 * O^occ * V^virt. Note that we only use the info from left, because it is a full contraction it will be the same as right.
See 0a237de. A new test case added which mimics CC energy expression.
@ajay-mk ajay-mk force-pushed the ajay/fix/asy-cost-scalar-result branch from 01d745b to 132b285 Compare February 2, 2026 05:05
@ajay-mk ajay-mk requested a review from Copilot February 2, 2026 05:07
@ajay-mk ajay-mk changed the title Handle full contraction to scalar in Flops computation Handle full contraction in Flops computation Feb 2, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates flop-cost computation to correctly account for tensor–tensor products that fully contract to a scalar (e.g., CC energy expressions), and adds a regression test covering this case.

Changes:

  • Extend Flops::operator() to handle tensor × tensor → scalar products.
  • Add a unit test for a CC energy-like scalar expression and its expected asymptotic cost.

Reviewed changes

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

File Description
SeQuant/core/eval/eval_node.hpp Adds a scalar-result path for tensor–tensor products in flop-cost estimation.
tests/unit/test_eval_node.cpp Adds a regression test validating scalar-result contractions and expected asy_cost.

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

In case this is not satisfied, we should catch it before, but just to be safe.
@ajay-mk ajay-mk requested a review from bimalgaudel February 2, 2026 06:24
@ajay-mk ajay-mk marked this pull request as ready for review February 2, 2026 06:26
Copy link
Member

@bimalgaudel bimalgaudel left a comment

Choose a reason for hiding this comment

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

This code:

  • has not been updated in long time
  • only assumes occupied and virtuals (unoccupied) orbital spaces
  • maybe should be deprecated..

@evaleev
Copy link
Member

evaleev commented Feb 2, 2026

This code:

  • has not been updated in long time
  • only assumes occupied and virtuals (unoccupied) orbital spaces
  • maybe should be deprecated..

@bimalgaudel Will the revisions in #462 make this obsolete?

@bimalgaudel
Copy link
Member

This code:

  • has not been updated in long time
  • only assumes occupied and virtuals (unoccupied) orbital spaces
  • maybe should be deprecated..

@bimalgaudel Will the revisions in #462 make this obsolete?

AsyCost will not be directly addressed, however, we will have a better (more efficient) mechanism to represent the symbolic cost in terms of all indices present rather than occ or virt. AsyCost class right now does not do much. So I suggest we drop it.

@ajay-mk
Copy link
Member Author

ajay-mk commented Feb 2, 2026

@evaleev @bimalgaudel Can I merge this now? We can deprecate or remove AsyCost in #462.

@bimalgaudel bimalgaudel merged commit 754ceb3 into master Feb 2, 2026
16 checks passed
@bimalgaudel bimalgaudel deleted the ajay/fix/asy-cost-scalar-result branch February 2, 2026 16:38
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.

4 participants