Savitzky–Golay derivative output omits `Deriv!` and sample spacing

[gabrielfrasantos]

Severity: high
Domain: filters
Status: VERIFIED — read + hand-traced against a210d34 on 2026-08-10
Suggested labels: bug, numerical-correctness, filters

Summary

The Savitzky–Golay kernel returns the fitted polynomial coefficient a_Deriv rather than the
derivative, which is Deriv! · a_Deriv. There is also no 1/hᵈ sample-spacing scaling. Output is
therefore only correct for Deriv ≤ 1 and unit spacing.

The same function additionally dereferences a std::optional without checking it.

Location

numerical/filters/passive/SavitzkyGolayFilter.hpp

Evidence

template<typename T, std::size_t Window, std::size_t Order, std::size_t Deriv>
constexpr std::array<T, Window> ComputeKernel()
{
    auto A = BuildVandermonde<T, Window, Order>();
    auto AtA = A.Transpose() * A;

    math::Vector<T, Order + 1> eDeriv{};
    eDeriv.at(Deriv, 0) = T{ 1 };

    auto z = math::CholeskyDecomposition<T, Order + 1>::Solve(AtA, eDeriv);

    std::array<T, Window> coeffs{};
    for (std::size_t k = 0; k < Window; ++k)
        for (std::size_t d = 0; d < Order + 1; ++d)
            coeffs[k] += A.at(k, d) * z->at(d, 0);      // (1) unchecked optional
                                                        // (2) missing Deriv! factor
    return coeffs;
}

Trace

p(j) = j², Deriv = 2. The least-squares fit recovers a₂ = 1, and the kernel returns 1.
The true second derivative is d²/dj² (j²) = 2.

returned:  a_Deriv         = 1
correct:   Deriv! · a_Deriv = 2! · 1 = 2

For Deriv = 3 the factor would be 6, and so on.

Two defects to fix

1. Missing factorial. Multiply the kernel by Deriv!.
2. Missing spacing. Divide by h^Deriv for a sample interval h, or document that the filter
   assumes unit spacing and returns derivatives per-sample.
3. Unchecked std::optional. z->at(...) is undefined behaviour if TryFactor returns
   nullopt. Although AᵀA for a Vandermonde matrix is normally SPD, this is a
   static inline const initialised at static-init time — a failure here is unrecoverable and
   silent. Check has_value() and static_assert/fail loudly.

Suggested fix

Add a compile-time Factorial<Deriv>() constant and apply it to the kernel; add a test asserting
the second derivative of j² is 2 and of j³ at the centre is 0.