Claude AI edits to TwoPuncturesX

[zachetienne]

Migrated from Bitbucket BB #2931.
Original reporter: Steven R. Brandt (GitHub profile)
Original assignee: unassigned
Original state: resolved; kind: bug; priority: major
Original created: 2026-04-16T15:53:12.091238+00:00; updated: 2026-05-16T03:12:22.634510+00:00

---

I ran Claude AI on the TwoPuncturesX thorn and it found errors which I believe to be legitimate. I am attaching Claude's report along with a PR implementing what it asks for.

---

Migrated attachments

• TwoPuncturesX_bug_report.md (4366 bytes)

[zachetienne]

Below message authored by: Erik Schnetter (GitHub profile)

Most of the reported problems are indeed problems that need correcting.

"3. Syntax errors in param.ccl" is wrong; this is not a syntax error.

"5. BY_KKofxyz missing epsilon regularization — Equations.cc:32–37" is probably also not an error; the source comment explicitly states that the result can be singular.

[zachetienne]

Below message authored by: Steven R. Brandt (GitHub profile)

EinsteinToolkit/SpacetimeX#64

[zachetienne]

Below message authored by: Steven R. Brandt (GitHub profile)

I put the correction for 5 in, but I can take it out.

[zachetienne]

Below message authored by: Erik Schnetter (GitHub profile)

Didn't you miss a few items? You didn't correct line 144, and you didn't put in a correction for 5 either (I think you shouldn't).

[zachetienne]

Below message authored by: Zach Etienne (GitHub profile)

Here is the report from Codex. It did not have access to Steve's review:

* Issue 1
 - Severity critical
 - Description of the error
   The code advertises a 4-variable momentum-constraint solve via `solve_momentum_constraint=yes`, but the equation kernels still only populate `values[0]`. The extra three variables therefore have no equations, and the resulting nonlinear system is mathematically ill-posed.
 - Filename + line number(s)
   `TwoPuncturesX/src/TwoPunctures.cc:40-41, 302-306`
   `TwoPuncturesX/src/Equations.cc:158-181`
 - Original code snippet
   if (solve_momentum_constraint)
     nvar = 4;

   values[0] = U.d11[0] + U.d22[0] + U.d33[0] +
               0.125 * BY_KKofxyz(x, y, z) / psi7 +
               2.0 * Pi / psi2 / psi * rho_adm;

   values[0] = dU.d11[0] + dU.d22[0] + dU.d33[0] -
               0.875 * BY_KKofxyz(x, y, z) / psi8 * dU.d0[0];

   SpecCoef(n1, n2, n3, 0, v.d0, cf_v.d0);
 - Fixed code snippet.
   if (solve_momentum_constraint) {
     CCTK_ERROR("solve_momentum_constraint=yes is not implemented in TwoPuncturesX");
   }

   /* Longer-term fix: implement and fill values[0..3] consistently, and
      generate/store spectral coefficients for all solved variables. */
 - Confidence
   99%

* Issue 2
 - Severity critical
 - Description of the error
   Turning on sources while leaving the default `rescale_sources=yes` triggers an unconditional `assert(0)`, so the source-enabled branch cannot run successfully.
 - Filename + line number(s)
   `TwoPuncturesX/src/TwoPunctures.cc:626-630`
   `TwoPuncturesX/param.ccl:162-164`
 - Original code snippet
   if (use_sources && rescale_sources) {
     assert(0); // TODO: Implement via critical region
   #pragma omp single
     Rescale_Sources(cctkGH, np, vcoordx, vcoordy, vcoordz, NULL, gxx, gyy, gzz,
                     gxy, gxz, gyz);
   }
 - Fixed code snippet.
   if (use_sources && rescale_sources) {
     Rescale_Sources(cctkGH, np, vcoordx, vcoordy, vcoordz, NULL, gxx, gyy, gzz,
                     gxy, gxz, gyz);
   }

   /* If thread-safety is not yet guaranteed, then the safe interim fix is to
      reject this configuration explicitly in ParamCheck instead of asserting. */
 - Confidence
   98%

* Issue 3
 - Severity high
 - Description of the error
   Four CCL parameter ranges have malformed syntax (`0.0:*)` instead of `(0.0:*)`), which can prevent correct parameter parsing.
 - Filename + line number(s)
   `TwoPuncturesX/param.ccl:100-118`
 - Original code snippet
   REAL par_m_plus "mass of the m+ puncture" STEERABLE = ALWAYS
   {
     0.0:*) :: ""
   } 1.0

   REAL par_m_minus "mass of the m- puncture" STEERABLE = ALWAYS
   {
     0.0:*) :: ""
   } 1.0

   REAL target_M_plus "target ADM mass for m+"
   {
     0.0:*) :: ""
   } 0.5

   REAL target_M_minus "target ADM mass for m-"
   {
     0.0:*) :: ""
   } 0.5
 - Fixed code snippet.
   REAL par_m_plus "mass of the m+ puncture" STEERABLE = ALWAYS
   {
     (0.0:*) :: ""
   } 1.0

   REAL par_m_minus "mass of the m- puncture" STEERABLE = ALWAYS
   {
     (0.0:*) :: ""
   } 1.0

   REAL target_M_plus "target ADM mass for m+"
   {
     (0.0:*) :: ""
   } 0.5

   REAL target_M_minus "target ADM mass for m-"
   {
     (0.0:*) :: ""
   } 0.5
 - Confidence
   97%

* Issue 4
 - Severity high
 - Description of the error
   In the `r_minus < TP_Extend_Radius` branch of the antisymmetric/averaged lapse, the denominator uses the plus-puncture mass where it should use the minus-puncture mass.
 - Filename + line number(s)
   `TwoPuncturesX/src/TwoPunctures.cc:605-607`
 - Original code snippet
   if (r_minus < TP_Extend_Radius) {
     alp[ind] = ((1.0 - 0.5 * EXTEND(*mm, r_minus) - 0.5 * *mp / r_plus) /
                 (1.0 + 0.5 * EXTEND(*mp, r_minus) + 0.5 * *mp / r_plus));
   }
 - Fixed code snippet.
   if (r_minus < TP_Extend_Radius) {
     alp[ind] = ((1.0 - 0.5 * EXTEND(*mm, r_minus) - 0.5 * *mp / r_plus) /
                 (1.0 + 0.5 * EXTEND(*mm, r_minus) + 0.5 * *mp / r_plus));
   }
 - Confidence
   96%

* Issue 5
 - Severity high
 - Description of the error
   The BiCGStab driver can exit because it exhausted `itmax` iterations, yet still fall through to the success return path because the failure check uses `ii > itmax` instead of `ii >= itmax`.
 - Filename + line number(s)
   `TwoPuncturesX/src/Newton.cc:330-435`
 - Original code snippet
   for (ii = 0; ii < itmax; ii++) {
     ...
   }

   /* iteration failed */
   if (ii > itmax)
     return -1;
 - Fixed code snippet.
   for (ii = 0; ii < itmax; ii++) {
     ...
   }

   /* iteration failed */
   if (ii >= itmax && *normres > tol)
     return -1;
 - Confidence
   95%

* Issue 6
 - Severity medium
 - Description of the error
   The initial-guess debug-output path calls `rx3_To_xyz()` with a stale `i3D` index instead of the current `indx`, which can corrupt the diagnostic coordinates written to `initial.dat`.
 - Filename + line number(s)
   `TwoPuncturesX/src/TwoPunctures.cc:101-103`
 - Original code snippet
   C_To_c(nvar, X, R, &(s_x[indx]), &r, U);
   rx3_To_xyz(nvar, s_x[i3D], r, phi, &(s_y[indx]), &(s_z[indx]), U);
 - Fixed code snippet.
   C_To_c(nvar, X, R, &(s_x[indx]), &r, U);
   rx3_To_xyz(nvar, s_x[indx], r, phi, &(s_y[indx]), &(s_z[indx]), U);
 - Confidence
   92%

====

I then gave Codex Steve's list and asked whether there was any overlap with what it found and Steve's list, and to identify any false positives. Here's what it said:

Only item 5 looks like a likely false positive to me. The others are real findings, though item 6 is phrased a bit more absolutely than I’d prefer.

1. Stale i3D in debug output

   Not a false positive. This is real.

   It is also a duplicate of my debug-indexing finding, with one useful addition: you correctly caught two occurrences, at lines around 103 and 144. My report only called out the first site, so this is a duplicate plus an extra instance of the same bug.

2. Wrong mass in antisymmetric lapse denominator

   Not a false positive. This is real.

   It is a duplicate of my lapse-denominator finding.

3. Syntax errors in param.ccl

   Not a false positive. This is real.

   It is a duplicate of my param.ccl syntax finding.

4. Off-by-one in bicgstab failure check

   Not a false positive. This is real.

   It is a duplicate of my ii > itmax / ii >= itmax finding.

5. BY_KKofxyz missing epsilon regularization

   This is the one I would treat as a false positive, or at least not established strongly enough to call a confirmed bug.

   Why:

   • BY_KKofxyz() is used in the spectral solve on collocation points.
   • In this formulation, the punctures sit at the boundary of the compactified domain, not at interior collocation points.
   • The collocation points are generated from
     A = -cos(pi/2 * (2i+1)/n1) and similarly for B, so they do not land exactly on the puncture location.
   • That means the unsmoothed 1/r behavior in BY_KKofxyz() is not automatically a runtime singularity in the Newton solve.

   So the claim “the nonlinear equation and Jacobian can blow up during solving if any collocation point lands at the puncture” is not supported by the actual collocation layout. It is still fair to note that BY_KKofxyz() and BY_Aijofxyz() are inconsistent in their regularization choices, but that is weaker than a confirmed bug.

   It is not a duplicate of anything I identified.

6. use_sources + rescale_sources hits assert(0)

   Not a false positive, but the wording is slightly too absolute.

   Why:

   • In a normal assert-enabled build, this is plainly a real bug/unimplemented feature trap.
   • Since rescale_sources defaults to "yes", use_sources = yes does indeed walk straight into that branch unless the user overrides it.
   • The only nuance is that assert(0) may disappear in a release build compiled with NDEBUG, so “will always abort” is a bit too strong as a universal statement.

   Still, as a thorn-quality issue this is absolutely valid.

   It is a duplicate of my assert(0) / rescale_sources finding.

Bottom line

• Likely false positive: #5
• Real duplicates of my findings: #2, #3, #4, #6
• Real partial duplicate of my findings: #1
  • I had the same bug, but not the second occurrence at line 144
• New and likely incorrect as a confirmed bug: #5

[zachetienne]

Below message authored by: Erik Schnetter (GitHub profile)

We clearly need to change our param.ccl syntax. It seems that mismatched parentheses are so uncommon that they are flagged as error. I wonder how many humans make the same mistake. Maybe we can allow square brackets instead of leaving out round parentheses?

[zachetienne]

Below message authored by: Steven R. Brandt (GitHub profile)

I say we leave it as is and wait for the clankers to become smart enough to figure it out.

[zachetienne]

Below message authored by: Steven R. Brandt (GitHub profile)

I think all clanker issues are now addressed. While there never was a passing test in this thorn, it still passes the test Lucas added in feature/TwoPuncturesX-tests.

[zachetienne]

Below message authored by: Steven R. Brandt (GitHub profile)

This PR addresses the issue: https://bitbucket.org/einsteintoolkit/einsteininitialdata/pull-requests/22

But through a new thorn, not SpacetimeX/TwoPuncturesX.

[zachetienne]

Below message authored by: Steven R. Brandt (GitHub profile)

No longer relevant.