Skip to content

Comments

[Derivatives] Implement forward-mode derivative for std::comp_ellint_1#1674

Open
ayush4874 wants to merge 16 commits intovgvassilev:masterfrom
ayush4874:feature/ellint-derivatives
Open

[Derivatives] Implement forward-mode derivative for std::comp_ellint_1#1674
ayush4874 wants to merge 16 commits intovgvassilev:masterfrom
ayush4874:feature/ellint-derivatives

Conversation

@ayush4874
Copy link

Summary

This PR implements the exact symbolic differentiation rule for std::comp_ellint_1 (Complete Elliptic Integral of the First Kind), resolving the fallback to numerical differentiation.

Fixes #1673

Mathematical Context

The derivative of the First Kind Elliptic Integral $K(k)$ is defined using the Second Kind Integral $E(k)$ as:

$$\frac{dK(k)}{dk} = \frac{E(k) - (1-k^2)K(k)}{k(1-k^2)}$$

where:

  • $K(k)$ maps to std::comp_ellint_1(k)
  • $E(k)$ maps to std::comp_ellint_2(k)

Implementation Details

  • File Modified: clad/Differentiator/BuiltinDerivatives.h
  • Changes:
    • Implemented comp_ellint_1_pushforward in the clad::custom_derivatives namespace.
    • Added the corresponding using declaration to register the function in the global lookup scope.
    • The implementation handles the C++17 standard library calls for elliptic integrals.

Verification

I tested the implementation locally by differentiating a wrapper function for std::comp_ellint_1.

1. Compilation Check:
The compilation warning falling back to numerical differentiation is gone, confirming Clad is now using the symbolic rule.

2. Numerical Accuracy:
I verified the derivative output at $k = 0.5$:

  • Analytical Value: 0.541733...
  • Clad Output: 0.541732

This confirms the implementation produces the correct gradients for physics applications.

@ayush4874
Copy link
Author

Hi! I checked the README to find the community chat, but I couldn't find a Gitter or Discord link. Is there an active channel I should join for development discussions?

using std::pow_pullback;
using std::pow_pushforward;
using std::sqrt_pushforward;
using std::comp_ellint_1_pushforward;
Copy link
Owner

Choose a reason for hiding this comment

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

We will need a pullback for this and also tests.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review!

I replaced the manual pullback with comp_ellint_1_darg0 (derivative w.r.t arg 0). This allows Clad to automatically derive the reverse-mode pullback, resolving the signature ambiguity.

I also added a regression test (clad/test/Differentiator/EllipticTest.cpp) and verified the gradients against the analytical values locally.

Verification Output:

Forward Derivative: 0.541732
Reverse Derivative: 0.541732

Comment on lines 1062 to 1066
// -------------------------------------------------------------------------
// [Ayush GSoC 2026 Contribution]
// Feature: Added support for Complete Elliptic Integral of the First Kind
// Target: std::comp_ellint_1(k)
// Formula: dK/dk = (E(k) - (1-k^2)*K(k)) / (k * (1-k^2))
Copy link
Owner

Choose a reason for hiding this comment

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

Remove the redundant comments.

@@ -0,0 +1,33 @@
// RUN: %clangxx -fplugin=%clad_plugin -std=c++17 %s -o %t
Copy link
Owner

Choose a reason for hiding this comment

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

This test should be part of test/Features/stl-cmath.cpp

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've moved the test case into test/Features/stl-cmath.cpp and removed the verbose comments from the header file.

//
DEFINE_FUNCTIONS(erf) // x in (-inf,+inf)

double comp_ellint_1_wrapper(double k) {
Copy link
Owner

Choose a reason for hiding this comment

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

You need to make this look the same as the other functions.

Copy link
Author

Choose a reason for hiding this comment

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

Done. Since comp_ellint_1 is a C++17 addition and does not have a legacy C equivalent (like comp_ellint_1f), I could not use the DEFINE_FUNCTIONS macro directly.

However, I have manually defined a templated wrapper f_comp_ellint_1 that follows the exact same naming convention and pattern as the macro, ensuring consistency with the rest of the file.

double comp_ellint_1_wrapper(double k) {
return std::comp_ellint_1(k);
}
// Manually defined to match DEFINE_FUNCTIONS pattern (comp_ellint_1f does not exist)
Copy link
Owner

Choose a reason for hiding this comment

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

Still does not look as the surroundings. Please make sure you understand this file and its comments.

Copy link
Author

Choose a reason for hiding this comment

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

My apologies for the oversight. I realized upon closer inspection that DEFINE_FUNCTIONS generates suffix variants (f_...f, f_...l) that my previous template missed.

I have now added f_comp_ellint_1f and f_comp_ellint_1l manually to fully replicate the macro's behavior as requested.

Comment on lines 301 to 302
inline float f_comp_ellint_1f(float x) { return std::comp_ellint_1(x); }
inline long double f_comp_ellint_1l(long double x) { return std::comp_ellint_1(x); }
Copy link
Owner

Choose a reason for hiding this comment

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

Why we cannot use DEFINE_FUNCTIONS here? Note this is missing the pullbacks and updating the section with the proper comment // ---- special fns. Also misses updating the table in the beginning of the file. Misses the range...

As I've said -- needs to look like the other functions tests in the same file.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated the file to fully match the surrounding style and structure as requested:

  1. Added comp_ellint_1 to the feature table at the top of the file.
  2. Moved the implementation to the "Special Functions" section with the // k in (-1, 1) range comment.
  3. Defined comp_ellint_1f and comp_ellint_1l wrappers so I can use DEFINE_FUNCTIONS(comp_ellint_1) directly.
  4. Updated the test case in main() to use the standard CHECK_ALL_RANGE macro.

Copy link
Author

Choose a reason for hiding this comment

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

@vgvassilev it's ready for review, can you have a look?

Copy link
Owner

Choose a reason for hiding this comment

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

Why adding only this special function and not the rest?

Copy link
Author

Choose a reason for hiding this comment

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

I initially isolated comp_ellint_1 to ensure the testing pattern for special functions was correct before expanding.

Since comp_ellint_2 (Complete Elliptic Integral of the Second Kind) is actually required for the derivative of comp_ellint_1, it is logical to group them. I will update this PR to include std::comp_ellint_2 and std::comp_ellint_3 so they can be reviewed together. I'll push those changes shortly.

Copy link
Author

Choose a reason for hiding this comment

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

Done, can you have a look?

Copy link
Contributor

@github-actions github-actions bot left a comment

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

}

// This allows Clad to auto-generate both Forward and Reverse mode.
CUDA_HOST_DEVICE double comp_ellint_1_darg0(double k) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'comp_ellint_1_darg0' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]

CUDA_HOST_DEVICE double comp_ellint_1_darg0(double k) {
                        ^
Additional context

include/clad/Differentiator/BuiltinDerivatives.h:1079: make as 'inline'

CUDA_HOST_DEVICE double comp_ellint_1_darg0(double k) {
                        ^

*d_y += (y / h) * d_z;
}

CUDA_HOST_DEVICE inline double comp_ellint_1_darg0(double k) {
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need the _darg versions here?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, we don't need them. I've removed the _darg helpers and moved the derivative logic directly inside the pushforward templates.

inline float comp_ellint_1f(float x) { return std::comp_ellint_1(x); }
inline long double comp_ellint_1l(long double x) { return std::comp_ellint_1(x); }

DEFINE_FUNCTIONS(comp_ellint_1)
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add the domain of x?

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've added comments specifying the domain is k in (-1, 1) for these integrals.

Copy link
Owner

Choose a reason for hiding this comment

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

look how was done for the other functions.

Comment on lines 321 to 333
double k = 0.5, nu = 0.3;

// Test differentiation w.r.t 'k' (Arg 0)
auto d_ellint3_k = clad::differentiate(std::comp_ellint_3, 0);
double res_k = d_ellint3_k.execute(k, nu);

// Test differentiation w.r.t 'nu' (Arg 1)
auto d_ellint3_nu = clad::differentiate(std::comp_ellint_3, 1);
double res_nu = d_ellint3_nu.execute(k, nu);

// Simple check to ensure we got numbers back (prevents unused var warning)
(void)res_k;
(void)res_nu;
Copy link
Owner

Choose a reason for hiding this comment

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

Here we should compare against the numerical, right? See what the CHECK macro does.

Copy link
Author

Choose a reason for hiding this comment

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

You are right. I updated the test to compare the symbolic result against a Finite Difference numerical approximation. The test will now fail (return 1) if the derivative deviates from the numerical reference.

inline float comp_ellint_1f(float x) { return std::comp_ellint_1(x); }
inline long double comp_ellint_1l(long double x) { return std::comp_ellint_1(x); }

DEFINE_FUNCTIONS(comp_ellint_1)
Copy link
Owner

Choose a reason for hiding this comment

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

look how was done for the other functions.

double res_k = d_ellint3_k.execute(k, nu);
double sym_k = d_ellint3_k.execute(k, nu);
// Numerical approximation: (f(k+h) - f(k-h)) / 2h
double num_k = (std::comp_ellint_3(k + h, nu) - std::comp_ellint_3(k - h, nu)) / (2 * h);
Copy link
Owner

Choose a reason for hiding this comment

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

There is an api in this file that gets you the numerical part.

Copy link
Author

Choose a reason for hiding this comment

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

I see the pattern now. I updated the tests to match atan2 and pow by using the f_ wrappers.

For comp_ellint_3, I fixed the second arg in the wrapper so I can just use CHECK_ALL_RANGE, which handles the numerical verification automatically.

Copy link
Owner

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM!

@vgvassilev vgvassilev force-pushed the feature/ellint-derivatives branch from 3944d20 to 2190f7b Compare February 3, 2026 09:46
@vgvassilev
Copy link
Owner

(assuming the bots are green)

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2026

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

@ayush4874
Copy link
Author

CI failures were caused by comp_ellint_1 (C++17) breaking the C++11/14 builds.

I’ve added #if __cplusplus >= 201703L guards around the implementation and using declarations in BuiltinDerivatives.h. The build should pass now.

@vgvassilev vgvassilev force-pushed the feature/ellint-derivatives branch from 523878c to c686d61 Compare February 3, 2026 18:08
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2026

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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2026

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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2026

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

@vgvassilev
Copy link
Owner

Hi @ayush4874, what is the state of this PR -- it seems to be failing on a lot of platforms.

@ayush4874
Copy link
Author

Hi @vgvassilev, apologies, I was busy with semester exams. I just pushed the fix for the CI failures.

The root cause was a type promotion mismatch. libstdc++ silently promotes float to double for these elliptic integrals. Since my _pushforward templates were strictly returning T, Clad rejected the signature, fell back to parsing the raw STL headers, and tripped over __builtin_isnan (which triggered the -Werror failures).

I fixed it by updating the templates with decltype and AdjOutType to catch the exact promoted return type. I also dropped the manual pullbacks for the 1-arg functions (comp_ellint_1 and 2) since Clad auto-derives them anyway, but I kept the explicit pullback for the multi-variable comp_ellint_3.

Tests are passing cleanly locally now.

@github-actions
Copy link
Contributor

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

@codecov
Copy link

codecov bot commented Feb 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ayush4874
Copy link
Author

Pushed a fix for the Mac CI failures. Older versions of libc++ don't support these math functions, so I added the proper macro guards. Formatting is fixed too.

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.

[Derivatives] Missing symbolic derivative for C++17 std::comp_ellint_1

2 participants