Skip to content

Fix min/max pushforwards in Hessian generation - #1885

Open
Elvand-Lie wants to merge 2 commits into
vgvassilev:masterfrom
Elvand-Lie:fix-issue-1872
Open

Fix min/max pushforwards in Hessian generation#1885
Elvand-Lie wants to merge 2 commits into
vgvassilev:masterfrom
Elvand-Lie:fix-issue-1872

Conversation

@Elvand-Lie

Copy link
Copy Markdown
Contributor

Summary

  • return std::min and std::max pushforward results by value
  • preserve exact custom-derivative matches first, then retry only eligible ValueAndPushforward<const T&, const T&> returns with value types
  • cover two-argument and explicit-comparator min/max Hessians

Fixes: #1872

Testing

  • Regressions/issue-1872.cpp
  • FirstDerivative/BuiltinDerivatives.C
  • ForwardMode/STLCustomDerivatives.C confirms reference-returning derivatives retain exact matching
  • full regression suite: 18 passed, 1 CUDA unsupported
  • changed-line clang-format --dry-run --Werror
  • git diff --check

Performance

Exact-HEAD versus patched, 11 alternating Release compile runs after warm-up:

  • STLCustomDerivatives.C: 1343.6 ms -> 1390.1 ms (+3.46%, overlapping ranges)
  • BuiltinDerivatives.C: 1307.6 ms -> 1271.4 ms (-2.77%)

At -O3, all four Hessian paths inline without ValueAndPushforward call or object overhead.

@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Return min/max pushforward results by value and retry custom derivative matching with value-typed ValueAndPushforward results only after exact const-reference matching fails.

Fixes: vgvassilev#1872

@github-actions github-actions Bot 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.

clang-tidy made some suggestions

Comment thread lib/Differentiator/DiffPlanner.cpp Outdated
// ValueAndPushforward<T, T> after the exact match fails. This keeps
// reference-returning custom derivatives preferred while allowing
// assignable min/max results in higher-order differentiation.
do {

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.

warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]

    do {
    ^

@vgvassilev
vgvassilev requested a review from guitargeek July 11, 2026 10:22
Comment thread lib/Differentiator/DiffPlanner.cpp Outdated
if (Expr* overload =
utils::MatchOverloadType(S, relaxedDTy, Found, RelaxedCandidates))
return overload;
} while (false);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This seems to be just a single iteration why the do-while-stmt?

@Elvand-Lie Elvand-Lie Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the do-while(false) wasn't intended to loop I intentionally used it as a breakable scope.

Because we have to fall through to the diagnostic code at the bottom on failure, using break allowed me to keep the early exit checks perfectly flat without deep nesting.

But after I noticed the code styling guide from Github actions I've now simplified the type-checking logic down to just three conditions, I've replaced it with a standard nested if in the latest push.

@github-actions

Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

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.

clad::hessian fails when std::max or similar functions are used

2 participants