Add IrrotationalBNS Pointwise Functions #5880

isaaclegred · 2024-03-21T22:56:06Z

Proposed changes

This PR adds the point wise functions necessary to compute the needed quantities for use in solving Irrotational BNS initial data (which will be added in a future pull request)

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 · 2024-03-29T18:49:04Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

+#include <cmath>
+#include <cstddef>
+
+#include "DataStructures/DataVector.hpp"
+#include "DataStructures/Tensor/Tensor.hpp"
+#include "PointwiseFunctions/GeneralRelativity/Lapse.hpp"
+#include "PointwiseFunctions/GeneralRelativity/Shift.hpp"
+
+#include "Utilities/Gsl.hpp"


#include "DataStructures/Tensor/TypeAliases.hpp" #include "Utilities/Gsl.hpp"

I don't think you need anything else.

wthrowe · 2024-03-29T18:49:58Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.cpp

+#include "PointwiseFunctions/GeneralRelativity/Lapse.hpp"
+#include "PointwiseFunctions/GeneralRelativity/Shift.hpp"


Remove these two.

wthrowe · 2024-03-29T18:52:37Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

+namespace hydro::initial_data {
+
+template <typename DataType>
+void rotational_shift(


These all need documentation.

Probably also worth adding an irrotational_bns namespace or something similar.

wthrowe · 2024-03-29T18:57:13Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.cpp

+    const Scalar<DataType>& local_angular_velocity_around_z,
+    const Scalar<DataType>& sqrt_det_spatial_metric) {
+  // Cross product involves volume element in arbitrary coordinates
+  result->get(0) = -get(sqrt_det_spatial_metric) * get<1>(x) *


get<0>(*result) and again below.

wthrowe · 2024-03-29T19:04:50Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.cpp

+                   get(local_angular_velocity_around_z);
+  result->get(1) = get(sqrt_det_spatial_metric) * get<0>(x) *
+                   get(local_angular_velocity_around_z);
+  result->get(2) = make_with_value<DataType>(sqrt_det_spatial_metric, 0.0);


For this function, use set_number_of_grid_points(result, sqrt_det_spatial_metric) (from Utilities/SetNumberOfGridPoints.hpp) at the start of the function and just assign a plain 0.0 to this component. Using make_with_value does an extra memory allocation and could break things if result was part of a Variables (and was therefore non-owning). For most of the other functions the tenex code will handle the resizing.

wthrowe · 2024-03-29T19:06:32Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.cpp

+    const tnsr::I<DataType, 3>& x,
+    const Scalar<DataType>& /*local_angular_velocity_around_z*/,
+    const Scalar<DataType>& /*sqrt_det_spatial_metric*/) {
+  *result = make_with_value<tnsr::iJ<DataType, 3>>(get<0>(x), 0.0);


Again, set_number_of_grid_points(result, x) and then = 0.0.

wthrowe · 2024-03-29T19:09:22Z

tests/Unit/PointwiseFunctions/Hydro/InitialData/CMakeLists.txt

@@ -0,0 +1,28 @@
+# Distributed under the MIT License.
+# See LICENSE.txt for details.
+ set(LIBRARY "Test_HydroInitialData")


Fix the formatting in this file.

wthrowe · 2024-03-29T19:13:11Z

tests/Unit/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.py

+    ) / lapse - np.outer(deriv_of_lapse / lapse**2, rotational_shift)
+
+
+def divergence_rotational_shift_stress(


The c++ part of the test for this function is missing.

Oh that function was removed because it became unnecessary, I'll remove it from the python functions, and from the IrrotationalBns.cpp file

wthrowe · 2024-03-29T19:14:18Z

tests/Unit/PointwiseFunctions/Hydro/InitialData/CMakeLists.txt

+  CoordinateMaps
+  DataStructures
+  DataStructuresHelpers
+  Domain
+  GeneralRelativity
+  GeneralRelativityHelpers
+  Hydro
+  HydroHelpers
+  LinearOperators
+  Spectral
+  IO
+  Utilities


Fix this list.

wthrowe

Not compiling.

wthrowe · 2024-04-02T19:12:22Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.cpp

@@ -172,7 +142,8 @@ void derivative_spatial_rotational_killing_vector(
    const tnsr::I<DataType, 3>& x,
    const Scalar<DataType>& /*local_angular_velocity_around_z*/,
    const Scalar<DataType>& /*sqrt_det_spatial_metric*/) {
-  *result = make_with_value<tnsr::iJ<DataType, 3>>(get<0>(x), 0.0);
+  set_number_of_grid_points(result, x);
+  tenex::update<ti::i, ti::J>(result, 0.0 * (*result)(ti::i, ti::J));


The values in result may be uninintialized or NaN, so you can't multiply by them. I don't know if we have a standard way to zero a Tensor, but std::fill(result->begin(), result->end(), 0.0) should work.

wthrowe · 2024-04-02T19:14:18Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

+/*!
+ * \ingroup HydroInitialDataGroup
+ * \brief Items related to solving for irrotational bns initial data
+ * See e.g. Baumgarte and Shapiro, Ch. 15 (P. 523)


\cite BaumgarteShapiro (It's already in our references file.)

wthrowe · 2024-04-02T19:14:54Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

 template <typename DataType>
 void rotational_shift(
    gsl::not_null<tnsr::I<DataType, 3>*> result,
    const tnsr::I<DataType, 3>& shift,
    const tnsr::I<DataType, 3>& spatial_rotational_killing_vector);
+/// @}


Move after the second overload.

wthrowe · 2024-04-02T19:16:16Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

+///   h^2 \rho^2 = \frac{1}{\alpha^2} \left(C + B^i D_i \Phi\right)^2 - D_i
+/// \Phi D^i \Phi
+/// \f]
+/// Where \f$\Phi \f$ is the velocity potential, andC is the Euler-constant,


Typo "andC" -> "and \f$C\f$"

wthrowe · 2024-04-02T19:26:18Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

+///
+/// The eqn. is identical to B&S 15.76
+/// \f[
+///   h^2 \rho^2 = \frac{1}{\alpha^2} \left(C + B^i D_i \Phi\right)^2 - D_i


Something is confusing doxygen around here, but I don't know exactly what. You might need to switch to /*! */ comments for multiline math.

wthrowe · 2024-04-02T19:29:00Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

+/// @{
+/// \brief The spatial derivative of the spatial rotational killing vector
+///
+/// As above, assumes uniform rotation around the z-axis


Doxygen doesn't do a great job of preserving organization, so "As above" ends up being confusing in the rendered documentation. Give the name of the specific thing you are trying to refer to. (I think "spatial_rotational_killing_vector".)

wthrowe · 2024-04-02T19:30:44Z

tests/Unit/PointwiseFunctions/Hydro/InitialData/CMakeLists.txt

@@ -0,0 +1,28 @@
+# Distributed under the MIT License.
+# See LICENSE.txt for details.
+set(LIBRARY "Test_HydroInitialData")


Add blank line above.

I think this was done before, but it seems to have gotten lost.

wthrowe · 2024-04-02T19:30:54Z

tests/Unit/PointwiseFunctions/Hydro/InitialData/CMakeLists.txt

+
+set(LIBRARY_SOURCES
+  Test_IrrotationalBns.cpp
+)


Indent two spaces.

wthrowe · 2024-04-02T19:31:03Z

tests/Unit/PointwiseFunctions/Hydro/InitialData/CMakeLists.txt

+add_test_library(${LIBRARY} "${LIBRARY_SOURCES}")
+
+
+


Remove extra blank lines.

wthrowe · 2024-04-02T19:34:36Z

tests/Unit/PointwiseFunctions/Hydro/InitialData/CMakeLists.txt

+  CoordinateMaps
+  DataStructures
+  DataStructuresHelpers
+  Domain
+  GeneralRelativity
+  GeneralRelativityHelpers
+  Hydro
+  HydroHelpers
+  IO
+  LinearOperators
+  Spectral
+  Utilities


All you use is

DataStructures Hydro

wthrowe · 2024-04-15T19:59:59Z

Looks good. Squash.

isaaclegred · 2024-04-16T18:58:28Z

@nilsvu if you'd like to take a look at this!

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

isaaclegred · 2024-04-30T16:25:51Z

@nilsvu this is ready to be looked at again when you get a chance!

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.cpp

nilsvu

You can squash everything

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.cpp

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.cpp

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

isaaclegred · 2024-05-02T22:36:38Z

@wthrowe if you'd like to take a look at this again, otherwise I think it's ready to merge

wthrowe · 2024-05-03T17:45:36Z

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp

+/// @{
+/// \brief Compute the spatial rotational killing vector associated with uniform
+/// rotation around the z-axis.
+/// Taking \f$\Omega_j\f$ to be the uniform rotation axis (assumed in the


Empty comment line after the "brief" part.

wthrowe · 2024-05-03T17:46:21Z

tests/Unit/PointwiseFunctions/Hydro/InitialData/CMakeLists.txt

@@ -0,0 +1,28 @@
+# Distributed under the MIT License.
+# See LICENSE.txt for details.
+set(LIBRARY "Test_HydroInitialData")


I think this was done before, but it seems to have gotten lost.

isaaclegred · 2024-05-06T16:40:57Z

@wthrowe shall I squash?

wthrowe · 2024-05-06T17:41:43Z

Yes, looks good.

isaaclegred · 2024-05-08T16:45:21Z

@wthrowe @nilsvu should be ready to go

isaaclegred force-pushed the BNSInitialDataFunctions branch from d21f030 to cec617b Compare March 26, 2024 00:20

wthrowe requested changes Mar 29, 2024

View reviewed changes

wthrowe requested changes Apr 2, 2024

View reviewed changes

isaaclegred force-pushed the BNSInitialDataFunctions branch from 5c1a257 to eb01613 Compare April 16, 2024 18:43

wthrowe previously approved these changes Apr 16, 2024

View reviewed changes

wthrowe requested a review from nilsvu April 18, 2024 19:14

nilsvu requested changes Apr 24, 2024

View reviewed changes

isaaclegred dismissed wthrowe’s stale review via c088295 April 29, 2024 22:40

nilsvu requested changes Apr 30, 2024

View reviewed changes

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp Outdated Show resolved Hide resolved

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.cpp Outdated Show resolved Hide resolved

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.cpp Show resolved Hide resolved

nilsvu requested changes Apr 30, 2024

View reviewed changes

isaaclegred force-pushed the BNSInitialDataFunctions branch from b7181af to 84e6569 Compare May 1, 2024 20:17

nilsvu requested changes May 2, 2024

View reviewed changes

src/PointwiseFunctions/Hydro/InitialData/IrrotationalBns.hpp Outdated Show resolved Hide resolved

isaaclegred force-pushed the BNSInitialDataFunctions branch from 84e6569 to b23ff80 Compare May 2, 2024 21:08

nilsvu previously approved these changes May 2, 2024

View reviewed changes

wthrowe requested changes May 3, 2024

View reviewed changes

isaaclegred dismissed nilsvu’s stale review via 9bb31c6 May 4, 2024 20:35

Add IrrotationalBNS Pointwise Functions

24a7cbe

isaaclegred force-pushed the BNSInitialDataFunctions branch from 9bb31c6 to 24a7cbe Compare May 7, 2024 16:31

nilsvu merged commit ebc0098 into sxs-collaboration:develop May 8, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IrrotationalBNS Pointwise Functions #5880

Add IrrotationalBNS Pointwise Functions #5880

isaaclegred commented Mar 21, 2024 •

edited

Loading

wthrowe Mar 29, 2024

wthrowe Mar 29, 2024

wthrowe Mar 29, 2024

wthrowe Mar 29, 2024

wthrowe Mar 29, 2024

wthrowe Mar 29, 2024

wthrowe Mar 29, 2024

wthrowe Mar 29, 2024

isaaclegred Mar 30, 2024

wthrowe Mar 29, 2024

wthrowe left a comment

wthrowe Apr 2, 2024

wthrowe Apr 2, 2024

wthrowe Apr 2, 2024

wthrowe Apr 2, 2024

wthrowe Apr 2, 2024

wthrowe Apr 2, 2024

wthrowe Apr 2, 2024

wthrowe May 3, 2024

wthrowe Apr 2, 2024

wthrowe Apr 2, 2024

wthrowe Apr 2, 2024

wthrowe commented Apr 15, 2024

isaaclegred commented Apr 16, 2024

isaaclegred commented Apr 30, 2024

nilsvu left a comment

isaaclegred commented May 2, 2024

wthrowe May 3, 2024

wthrowe May 3, 2024

isaaclegred commented May 6, 2024

wthrowe commented May 6, 2024

isaaclegred commented May 8, 2024

		#include "PointwiseFunctions/GeneralRelativity/Lapse.hpp"
		#include "PointwiseFunctions/GeneralRelativity/Shift.hpp"

		) / lapse - np.outer(deriv_of_lapse / lapse**2, rotational_shift)


		def divergence_rotational_shift_stress(

		add_test_library(${LIBRARY} "${LIBRARY_SOURCES}")

Add IrrotationalBNS Pointwise Functions #5880

Add IrrotationalBNS Pointwise Functions #5880

Conversation

isaaclegred commented Mar 21, 2024 • edited Loading

Proposed changes

Upgrade instructions

Code review checklist

Further comments

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

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 commented Apr 15, 2024

isaaclegred commented Apr 16, 2024

isaaclegred commented Apr 30, 2024

nilsvu left a comment

Choose a reason for hiding this comment

isaaclegred commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaaclegred commented May 6, 2024

wthrowe commented May 6, 2024

isaaclegred commented May 8, 2024

isaaclegred commented Mar 21, 2024 •

edited

Loading