Compactify the Scalar SF by NamiNishimura73 · Pull Request #7469 · sxs-collaboration/spectre

NamiNishimura73 · 2026-07-30T10:26:18Z

Proposed changes

Compactify the radial domain for Circular Scalar SF
Inside the u-region, x coordinate is defined as $\sigma = 2 r_u - \frac{r_u^2}{r}$.
Details are in CircularOrbit.hpp

Upgrade instructions

Code review checklist

The code is documented and the documentation renders correctly. Run
make doc to generate the documentation locally into BUILD_DIR/docs/html.
Then open index.html.
The code follows the stylistic and code quality guidelines listed in the
code review guide.
The PR lists upgrade instructions and is labeled bugfix or
new feature if appropriate.
If a coding agent was used, have a co-author trailer of the form
"Co-Authored-By: " as the last line of the
commit, e.g. "Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com",
"Co-Authored-by: Codex noreply@openai.com", or
"Co-Authored-By: GitHub Copilot CLI noreply@microsoft.com".

Further comments

NamiNishimura73 · 2026-07-30T10:40:03Z

+    // 2 r_u - r_u^2/r to compactify the wavezone.
+    // Elliptic PDEs will be re-written as a function of
+    // inv_r (1/r) instead of r.
+    inv_r = (2.0 * r_u - r) / (r_u * r_u);


This equation was derived as $\sigma = 2 r_u - \frac{r_u^2}{r}$, $\rightarrow$ $\frac{1}{r}=\frac{2r_{u}-\sigma}{r^{2}_{u}}$.
x-coordinate r in $u$-region represents $\sigma$ (this $\sigma$ is only used in this line, so I did not re-define. Let me know if it is too confusing.

Thanks, this comment was very helpful :D Please do expand the code comment a bit to make this clearer, and/or define sigma = get<0>(x) (see also comment above).

I added more detail in the explanation. Also defined sigma, directly from the x coordinate.

NamiNishimura73 · 2026-07-30T10:41:25Z

 *   advantages: first, the horizon is placed at a finite radius rather than
 *   $r_\star \rightarrow -\infty$; second, the flux component normal to the
- *   boundary vanishes at the horizon ($r=r_{\plus}$), which reduces to
+ *   boundary vanishes at the horizon ($r=r_+$), which reduces to


Initially I had r_{\plus}, which appears as "undefined" in the documentation, so this has been changed. (irrelevant to this PR)

NamiNishimura73 · 2026-07-30T10:41:34Z

+ * where $\Delta\phi(r) = \frac{a}{r_+ - r_-}
+ * \ln(\frac{r-r_+}{r-r_-})$.


Initially I had r_{\plus}, which appears as "undefined" in the documentation, so this has been changed. (irrelevant to this PR)

NamiNishimura73 · 2026-07-30T10:41:59Z

 * \begin{align}
 *   \Psi_m^P &= \frac{r}{2 \pi \sin(\theta)^{|m|}}
- *     e^{i m \left( \varphi - \phi\right)} \Phi_m^\mathrm{Wardell} \\
+ *     e^{-i m \left( \varphi - \phi\right)} \Phi_m^\mathrm{Wardell} \\


Cillian pointed out that I had typo in this equation. This has been fixed (irrelevant to this PR)

NamiNishimura73 · 2026-07-30T10:43:00Z

-            MModeNumber: *m_mode_number
-            HyperboloidalSlicing: True
-            Order: 2
+        Upper: None


Outer BC was changed as we no longer need to impose anything

NamiNishimura73 · 2026-07-30T10:45:28Z

  }
 }
+
+SPECTRE_TEST_CASE(


This test checks that, elliptic coefficient (in v or t region) modified by Jacobian returns to the elliptic coefficient in u region. Might be useful if we change elliptic coefficients in the future.
(But I am not sure if this test is necessary)

NamiNishimura73 · 2026-07-30T10:50:16Z

@nilsvu If you could take a look when you have time, that would be great, thanks!

NamiNishimura73 · 2026-08-09T03:02:31Z

For elliptic coefficients, I re-wrote them as a function of inv_r (=1/r) in the entire region (not just u-region) to simplify the expressions. This is to avoid defining elliptic coefficients differently whether they are in u-region or else where.

nilsvu · 2026-08-18T15:10:49Z

+    // 2 r_u - r_u^2/r to compactify the wavezone.
+    // Elliptic PDEs will be re-written as a function of
+    // inv_r (1/r) instead of r.
+    inv_r = (2.0 * r_u - r) / (r_u * r_u);


Thanks, this comment was very helpful :D Please do expand the code comment a bit to make this clearer, and/or define sigma = get<0>(x) (see also comment above).

nilsvu · 2026-08-18T15:28:13Z

I believe this code will be wrong in the u region. Is this a problem? It's probably better not to perform BL-to-tortoise transforms where $r_{BL}$ is actually $\sigma$. Maybe define sigma, inv_r etc. here properly, rather than below.

I removed r_star completely, as it was never used when penetrating_horizon is true (this is why the problem was never raised, even though the definition was wrong)

nilsvu · 2026-08-18T15:37:51Z

-    get<0>(alpha) = delta / r_sq_plus_a_sq;
-    get<1>(alpha) = 1.0 / r_sq_plus_a_sq;
+    get<0>(alpha) = (delta_over_r_sq / one_plus_a_sq_inv_r_sq) * dsigma_dr;
+    get<1>(alpha) = (square(inv_r) / one_plus_a_sq_inv_r_sq) / dsigma_dr;


I think it's better to be a bit more careful here with dsigma_dr -> 0. I think it would be reasonable to have in_u_region branches that take care of cancellations (where needed), rather than force everything into the 1/r form.

Yes I agree. thanks for catching this issue. When outside of u-region, I kept the original expression (without 1/r reformulation) and inside u-region, division by dsigma_dr is now taken care of, by some cancellation so that we no longer have 1/0 in u-region.

nilsvu · 2026-08-31T08:10:36Z

@@ -104,8 +104,6 @@ CircularOrbit::variables(
    // NOLINTNEXTLINE
    r.set_data_ref(const_cast<DataVector*>(&r_star_or_r));


This line is incorrect in the u-region where get<0>(x) is actually sigma. This means that many of the computations below are incorrect (like delta etc). I think you're careful about only using correct computations in if (in_u_region) branches, but this is still confusing and easy to introduce bugs. Can you define sigma here properly and make sure all computations below are correct, or (if this makes the code too complicated or leads to unnecessary computations) add comments to clarify exactly which quantities are computed correctly in which region?

You are right. Though they were never used in u-region, it was confusing and misleading. I re-defined r and sigma when penetrating_horizon is true, so that computations below (delta, etc) are always true.

NamiNishimura73 commented Jul 30, 2026

View reviewed changes

NamiNishimura73 marked this pull request as ready for review July 30, 2026 10:49

Compactify the Scalar SF

a2938f3

NamiNishimura73 force-pushed the Scalar_SF_compactification branch from 24e6d91 to a2938f3 Compare August 9, 2026 03:00

nilsvu requested changes Aug 18, 2026

View reviewed changes

Addressing review comments

d680ba8

NamiNishimura73 force-pushed the Scalar_SF_compactification branch from 78db2fa to d680ba8 Compare August 25, 2026 01:53

nilsvu requested changes Aug 31, 2026

View reviewed changes

addressed comments 2

72ae8a2

		* where $\Delta\phi(r) = \frac{a}{r_+ - r_-}
		* \ln(\frac{r-r_+}{r-r_-})$.

		@@ -104,8 +104,6 @@ CircularOrbit::variables(
		// NOLINTNEXTLINE
		r.set_data_ref(const_cast<DataVector*>(&r_star_or_r));

Uh oh!

Conversation

NamiNishimura73 commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Upgrade instructions

Code review checklist

Further comments

Uh oh!

NamiNishimura73 Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NamiNishimura73 commented Jul 30, 2026

Uh oh!

NamiNishimura73 commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NamiNishimura73 commented Jul 30, 2026 •

edited

Loading

NamiNishimura73 Jul 30, 2026 •

edited

Loading

NamiNishimura73 commented Aug 9, 2026 •

edited

Loading