Skip to content

Fix swapped location indices in UnivariateMaternNuggetsStationary covariance#58

Open
fonzie42 wants to merge 1 commit into
ecrc:mainfrom
fonzie42:bugfix/matern-nuggets-distance-index-order
Open

Fix swapped location indices in UnivariateMaternNuggetsStationary covariance#58
fonzie42 wants to merge 1 commit into
ecrc:mainfrom
fonzie42:bugfix/matern-nuggets-distance-index-order

Conversation

@fonzie42

@fonzie42 fonzie42 commented Jun 28, 2026

Copy link
Copy Markdown

Summary

UnivariateMaternNuggetsStationary::GenerateCovarianceMatrix passes the two
location indices to CalculateDistance in the wrong order, (j0, i0) instead
of (i0, j0). Every other kernel in the project passes (i0, j0).

Root cause

The distance helper signature is:

CalculateDistance(Locations &loc1, Locations &loc2, idxInto_loc1, idxInto_loc2, ...)

In the generation loop, i0 walks the rows (indexing aLocation1) and j0
walks the columns (indexing aLocation2), so the correct call is
(..., i0, j0, ...). The nuggets kernel instead calls (..., j0, i0, ...) in
both branches.

This is invisible for the symmetric train-train covariance matrix, because
the Euclidean distance is symmetric: d(loc[i], loc[j]) == d(loc[j], loc[i]).
That is why synthetic data generation and MLE are unaffected, and why the
existing kernel test (which only checks the symmetric matrix) never caught it.

It does corrupt the rectangular cross-covariance matrix C12 assembled
during prediction, where the two location sets (observed and missing) differ.
Entry (i, j) ends up using d(location1[j], location2[i]) instead of
d(location1[i], location2[j]), the transposed matrix, producing wrong
kriging predictions whenever this kernel is used for prediction.

Fix

-                expr = DistanceCalculationHelpers<T>::CalculateDistance(aLocation1, aLocation2, j0, i0, aDistanceMetric,
+                expr = DistanceCalculationHelpers<T>::CalculateDistance(aLocation1, aLocation2, i0, j0, aDistanceMetric,

(applied to both branches of GenerateCovarianceMatrix).

Test

Adds TestUnivariateMaternNuggetsCrossCovariance.cpp: it builds a 3×3
cross-covariance between two distinct location sets and checks every entry
against an independently computed reference. With nu = 0.5 the Matérn
covariance reduces to the exponential covariance sigma^2 * exp(-d / beta), so
the reference values are self-evident and do not depend on any internal index
convention.

  • With the swapped indices, entry (0,1) resolves to exp(-1) ≈ 0.36788 (the
    transposed pair, distance 1) instead of exp(-3) ≈ 0.04979 (distance 3), so
    the test distinguishes the two index orders.
  • With the corrected indices, every entry matches the reference.
  • The existing symmetric kernel test is unaffected, since the symmetric
    (data-generation / MLE) path does not depend on the index order.

Note

Companion fix: the R-interface prediction problem-size fix (#59). The
two are independent code changes (disjoint files); both are needed together for
fully correct end-to-end kriging predictions with the nuggets kernel.

…getsStationary

GenerateCovarianceMatrix passed (j0, i0) to CalculateDistance instead of
(i0, j0), swapping which location set each loop index addresses. Every other
kernel in the project passes (i0, j0).

The defect is invisible for the symmetric train-train covariance matrix
(Euclidean distance is symmetric, so d(loc[i], loc[j]) == d(loc[j], loc[i]))
which is why data generation and MLE are unaffected. It does corrupt the
rectangular cross-covariance matrix C12 assembled during prediction, where the
two location sets (observed and missing) differ: entry (i, j) ends up using
d(location1[j], location2[i]) instead of d(location1[i], location2[j]),
producing the transposed matrix and therefore wrong kriging predictions.

Add an isolated regression test that builds a 3x3 cross-covariance between two
distinct location sets and checks every entry against an independently computed
reference (Matern nu=0.5 reduces to sigma^2 * exp(-d/beta)). On the old code
entry (0,1) returns exp(-1) instead of exp(-3); with the fix all entries match.
The existing data-generation test for this kernel continues to pass.
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.

2 participants