Replace Kerr-Schild with Spherical-Kerr-Schild for FishboneMoncriefDisk #5800

jyoo1042 · 2024-02-23T18:13:23Z

Proposed changes

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.

Further comments

wthrowe

I'm confused about what is going on with the mass and spin. It seems that some places are being hardcoded to 1 and 0, but not all places.

wthrowe · 2024-02-23T18:26:31Z

src/Evolution/Executables/GrMhd/ValenciaDivClean/EvolveValenciaDivClean.hpp

@@ -270,7 +272,7 @@ struct EvolutionMetavars<tmpl::list<InterpolationTargetTags...>,
                  ::Events::Tags::ObserverMesh<volume_dim>,
                  ::Events::Tags::ObserverInverseJacobian<
                      volume_dim, Frame::ElementLogical, Frame::Inertial>>>,
-          error_tags,
+          error_tags, dt_tags,


Observing the time derivatives is a bad idea. You don't get the current value, but rather whatever the last RHS evaluation was, which was probably at a different time and possibly on a substep.

removed this commit.

wthrowe · 2024-02-23T18:33:49Z

src/PointwiseFunctions/AnalyticSolutions/GeneralRelativity/SphericalKerrSchild.hpp

+      tmpl::push_back<tags<DataType, Frame>,
+                      typename internal_tags::inv_jacobian<DataType, Frame>>;
+
+  // Overload and call the second one.


I don't understand this comment.

wthrowe · 2024-02-23T18:35:22Z

src/PointwiseFunctions/AnalyticData/GrMhd/MagnetizedFmDisk.cpp

+      // to do transformation below. copy x into x_sks and alter at each
+      // element index s.
+      const double sks_to_ks_factor =
+          sqrt(r_squared + square(fm_disk_.bh_spin_a_)) / radius;


Drop spin term? You seem to be changing it to zero everywhere else.

This is correct. I was confused before.

wthrowe · 2024-02-23T18:39:07Z

src/PointwiseFunctions/AnalyticData/GrMhd/MagnetizedFmDisk.cpp

+      for (size_t i = 0; i < 3; ++i) {
+        get_element(magnetic_field_sks.get(j), s) +=
+            get_element(inv_jacobians.get(j, i), s) *
+            get_element(magnetic_field_ks.get(i), s);


This doesn't need to be done pointwise.

changed it.

wthrowe · 2024-02-23T18:45:43Z

tests/Unit/PointwiseFunctions/AnalyticData/GrMhd/Test_MagnetizedFmDisk.cpp

@@ -111,19 +111,21 @@ void test_serialize() {

 template <typename DataType>
 void test_variables(const DataType& used_for_size) {
-  const double bh_mass = 1.12;
+  //   const double bh_mass = 1.12;
+  const double bh_mass = 1.;


Revert? Drop comment?

dropped the comment and changed the mass from 1.

wthrowe · 2024-02-23T18:46:24Z

tests/Unit/PointwiseFunctions/AnalyticData/GrMhd/MagnetizedFmDisk.py

+        # normalization resolution for a faster testing.
+        # normalization = 1.71896376101037762218  # 51 before the change
+        normalization = 1.3733086119266160185503622  # 51 after the change
+        # normalization = 1.3686080052126354811292686 # 255 after the change.


"the change"?

wthrowe · 2024-02-23T18:47:17Z

tests/Unit/PointwiseFunctions/AnalyticData/GrMhd/Test_MagnetizedFmDisk.cpp

-  // Test a few of the GR components to make sure that the implementation
-  // correctly forwards to the background solution of the base class.
+  //   // Test a few of the GR components to make sure that the implementation
+  //   // correctly forwards to the background solution of the base class.


Change back.

wthrowe · 2024-02-23T18:50:21Z

tests/Unit/PointwiseFunctions/AnalyticData/GrMhd/Test_MagnetizedFmDisk.cpp

-      Catch::Matchers::ContainsSubstring(
-          "Value 2 is below the lower bound of 4"));
+  #ifdef SPECTRE_DEBUG
+    CHECK_THROWS_WITH(


Something went wrong with the formatting in this section. The original version looked correct.

Are you talking about the extra indentation?

wthrowe · 2024-02-23T18:56:01Z

src/PointwiseFunctions/AnalyticSolutions/RelativisticEuler/FishboneMoncriefDisk.cpp

+               get<gr::Tags::Shift<DataType, 3>>(
+                   background_spacetime_.variables(
+                       x, 0.0, tmpl::list<gr::Tags::Shift<DataType, 3>>{},
+                       make_not_null(&vars->sph_kerr_schild_cache)))


Compute this outside the loop. I think it can even go outside the lambda.

Ping. Also move inv_jacobians, now that I look harder.

wthrowe · 2024-02-23T18:57:53Z

src/PointwiseFunctions/AnalyticSolutions/RelativisticEuler/FishboneMoncriefDisk.cpp

+          get_element(get(get<gr::Tags::Lapse<DataType>>(
+                          background_spacetime_.variables(
+                              x, 0.0, tmpl::list<gr::Tags::Lapse<DataType>>{},
+                              make_not_null(&vars->sph_kerr_schild_cache)))),


Same as for the shift.

wthrowe · 2024-02-27T22:50:08Z

src/PointwiseFunctions/AnalyticData/GrMhd/MagnetizedFmDisk.cpp

+      // to do transformation below. copy x into x_sks and alter at each
+      // element index s.
+      const double sks_to_ks_factor =
+          sqrt(r_squared + square(fm_disk_.bh_spin_a_)) / radius;


This is correct. I was confused before.

wthrowe · 2024-02-27T23:00:23Z

src/PointwiseFunctions/AnalyticSolutions/RelativisticEuler/FishboneMoncriefDisk.cpp

+               get<gr::Tags::Shift<DataType, 3>>(
+                   background_spacetime_.variables(
+                       x, 0.0, tmpl::list<gr::Tags::Shift<DataType, 3>>{},
+                       make_not_null(&vars->sph_kerr_schild_cache)))


Ping. Also move inv_jacobians, now that I look harder.

wthrowe · 2024-02-27T23:00:44Z

src/PointwiseFunctions/AnalyticSolutions/RelativisticEuler/FishboneMoncriefDisk.cpp

+          get_element(get(get<gr::Tags::Lapse<DataType>>(
+                          background_spacetime_.variables(
+                              x, 0.0, tmpl::list<gr::Tags::Lapse<DataType>>{},
+                              make_not_null(&vars->sph_kerr_schild_cache)))),


wthrowe · 2024-02-27T23:05:14Z

tests/Unit/PointwiseFunctions/AnalyticData/GrMhd/Test_MagnetizedFmDisk.cpp

-      Catch::Matchers::ContainsSubstring(
-          "Value 2 is below the lower bound of 4"));
+  #ifdef SPECTRE_DEBUG
+    CHECK_THROWS_WITH(


FM_disk

wthrowe

Please push fixup commits in the future.

jyoo1042 requested a review from wthrowe February 23, 2024 18:13

wthrowe requested changes Feb 23, 2024

View reviewed changes

jyoo1042 closed this Feb 23, 2024

jyoo1042 reopened this Feb 27, 2024

jyoo1042 force-pushed the fishy_PR branch from cc7d49d to 15c9b2c Compare February 27, 2024 19:26

wthrowe requested changes Feb 27, 2024

View reviewed changes

jyoo1042 added 2 commits February 28, 2024 10:00

Fix a typo in Spherical KS Documentation

adb4525

Replace KerrSchild with SphericalKerrSchild as background spacetime for

31ee7ab

FM_disk

jyoo1042 force-pushed the fishy_PR branch from 15c9b2c to 31ee7ab Compare February 28, 2024 15:25

wthrowe approved these changes Feb 28, 2024

View reviewed changes

wthrowe merged commit 8126111 into sxs-collaboration:develop Feb 28, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Kerr-Schild with Spherical-Kerr-Schild for FishboneMoncriefDisk #5800

Replace Kerr-Schild with Spherical-Kerr-Schild for FishboneMoncriefDisk #5800

jyoo1042 commented Feb 23, 2024

wthrowe left a comment

wthrowe Feb 23, 2024

jyoo1042 Feb 27, 2024

wthrowe Feb 23, 2024

wthrowe Feb 23, 2024

wthrowe Feb 27, 2024

wthrowe Feb 23, 2024

jyoo1042 Feb 27, 2024

wthrowe Feb 23, 2024

jyoo1042 Feb 28, 2024

wthrowe Feb 23, 2024

wthrowe Feb 23, 2024

wthrowe Feb 23, 2024

jyoo1042 Feb 27, 2024

wthrowe Feb 27, 2024

jyoo1042 Feb 28, 2024

wthrowe Feb 23, 2024

wthrowe Feb 27, 2024

jyoo1042 Feb 28, 2024

wthrowe Feb 23, 2024

wthrowe Feb 27, 2024

jyoo1042 Feb 28, 2024

wthrowe Feb 27, 2024

wthrowe Feb 27, 2024

wthrowe Feb 27, 2024

wthrowe Feb 27, 2024

wthrowe left a comment

Replace Kerr-Schild with Spherical-Kerr-Schild for FishboneMoncriefDisk #5800

Replace Kerr-Schild with Spherical-Kerr-Schild for FishboneMoncriefDisk #5800

Conversation

jyoo1042 commented Feb 23, 2024

Proposed changes

Upgrade instructions

Code review checklist

Further comments

wthrowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wthrowe left a comment

Choose a reason for hiding this comment