Add InverseJacobianInertialToFluidCompute #5811

ffoucart · 2024-02-27T23:15:59Z

Proposed changes

Implement a ComputeTag that calculates the inverse jacobian of the map between the inertial frame and an orthonormal tetrad comoving with the fluid.

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

I needed to add a new frame (Frame::Fluid) -- should it be in the same file as the other frames, or defined separately in the MC code?

ffoucart · 2024-02-28T18:13:44Z

Changed the accuracy required in the test; the default worked in Release mode on wheeler (at least every time I tried it) but consistently failed in Debug mode and for many of the configurations run by Jenkins.

wthrowe · 2024-02-28T19:33:38Z

src/Evolution/Particles/MonteCarlo/InverseJacobianInertialToFluidCompute.hpp

+                 gr::Tags::SpatialMetric<DataVector, 3> >;
+
+  static void function(gsl::not_null<return_type*> inv_jacobian,
+                const tnsr::I<DataVector, 3>& spatial_velocity,


Fix formatting.

wthrowe · 2024-02-28T19:36:38Z

src/Evolution/Particles/MonteCarlo/InverseJacobianInertialToFluidCompute.cpp

+        (spatial_velocity.get(d) - shift.get(d) / get(lapse));
+  }
+
+  // Then, the other members of the tetrad are constructed using Gram-Shmidt


wthrowe · 2024-02-28T19:41:36Z

src/Evolution/Particles/MonteCarlo/InverseJacobianInertialToFluidCompute.cpp

+  // Temporary memory allocation
+  DataVector temp_dot_product = get(lapse) * 0.0;
+  std::array<DataVector, 4> tetrad_vector{temp_dot_product, temp_dot_product,
+                                          temp_dot_product, temp_dot_product};


Instead of using this temporary, write the result directly into the inv_jacobian matrix. That saves four memory allocations, which is good for both speed and memory use.

wthrowe · 2024-02-28T19:44:36Z

src/Evolution/Particles/MonteCarlo/InverseJacobianInertialToFluidCompute.cpp

+  // Then, the other members of the tetrad are constructed using Gram-Shmidt
+
+  // Temporary memory allocation
+  DataVector temp_dot_product = get(lapse) * 0.0;


auto temp_dot_product = make_with_value<DataVector>(lapse, 0.0);

wthrowe

Other changes look good. Squash, including this last request.

wthrowe · 2024-02-29T20:25:01Z

src/Evolution/Particles/MonteCarlo/InverseJacobianInertialToFluidCompute.cpp


  // d = 1,2,3 for tetrad components built from x,y,z
  for (size_t d = 1; d < 4; d++) {
    // Base vector for Gram-Shmidt
    for (size_t i = 0; i < 4; i++) {
-      gsl::at(tetrad_vector, i) = (i == d ? 1.0 : 0.0);
+      inv_jacobian->get(i, d) = make_with_value<DataVector>(lapse, 0.0);


Do set_number_of_grid_points(&inv_jacobian->get(i, d), lapse) (from Utilities/SetNumberOfGridPoints.hpp) and then = 0.0 (or the ?: expression you had before). This avoids a DataVector allocation. Might need a make_not_null.

ffoucart · 2024-02-29T20:57:56Z

Squashed with last requested change.

wthrowe

Test failed in CI:

2024-02-29T23:23:33.9252167Z Unit.Evolution.Particles.MonteCarlo.InverseJacobianInertialToFluid
2024-02-29T23:23:33.9253216Z -------------------------------------------------------------------------------
2024-02-29T23:23:33.9255545Z ../../../../../../tests/Unit/Evolution/Particles/MonteCarlo/Test_InverseJacobianInertialToFluid.cpp:22
2024-02-29T23:23:33.9256689Z ...............................................................................
2024-02-29T23:23:33.9257210Z 
2024-02-29T23:23:33.9257566Z ../../../../../../tests/Unit/Framework/TestingFramework.hpp:116: FAILED:
2024-02-29T23:23:33.9258285Z   CHECK( a == appx(b) )
2024-02-29T23:23:33.9258704Z with expansion:
2024-02-29T23:23:33.9259514Z   -0.00000000000212888 == Approx( 0.0 )
2024-02-29T23:23:33.9260039Z with messages:
2024-02-29T23:23:33.9260650Z   Seed is: 2708572239 from ../../../../../../tests/Unit/Evolution/Particles/
2024-02-29T23:23:33.9261452Z   MonteCarlo/Test_InverseJacobianInertialToFluid.cpp:26
2024-02-29T23:23:33.9262207Z   ../../../../../../tests/Unit/Evolution/Particles/MonteCarlo/
2024-02-29T23:23:33.9263027Z   Test_InverseJacobianInertialToFluid.cpp:88: dot_product ==
2024-02-29T23:23:33.9263727Z   expected_dot_product
2024-02-29T23:23:33.9264480Z   a := (-1.55761e-14,3.35154e-17,1.85832e-16,-1.83013e-16,-2.12888e-12)
2024-02-29T23:23:33.9265165Z   b := (0,0,0,0,0)
2024-02-29T23:23:33.9265402Z 
2024-02-29T23:23:33.9265916Z 0.001 s: Unit.Evolution.Particles.MonteCarlo.InverseJacobianInertialToFluid
2024-02-29T23:23:33.9266854Z ===============================================================================
2024-02-29T23:23:33.9267485Z test cases:   1 |   0 passed | 1 failed
2024-02-29T23:23:33.9268385Z assertions: 127 | 126 passed | 1 failed

ffoucart · 2024-03-01T04:16:29Z

Is there a way to force the use of that seed in the test locally?
I could just raise the tolerance a bit (it fails the comparison by 2e-12 when the tolerance is 1e-12), but unless it's somehow using weird random values for the metric/velocity, I'd expect higher accuracy.

wthrowe · 2024-03-01T19:09:28Z

2024-02-29T23:23:33.9260650Z   Seed is: 2708572239 from ../../../../../../tests/Unit/Evolution/Particles/
2024-02-29T23:23:33.9261452Z   MonteCarlo/Test_InverseJacobianInertialToFluid.cpp:26

That line should have a MAKE_GENERATOR call on it. That macro takes an optional second argument of the seed.

ffoucart · 2024-03-01T19:29:55Z

I changed the epsilon from 1e-12 to 5e-12.
The failing point had a Lorentz factor of 20.8. The maximum allowed by the RNG is 21.1, and it makes sense that the calculation would loose accuracy for high Lorentz factors so... in my view it's ok for now.

wthrowe

OK, sounds good.

ffoucart force-pushed the InverseJacobianInertialToFluid branch from c2a263b to 9304927 Compare February 28, 2024 18:05

wthrowe requested changes Feb 28, 2024

View reviewed changes

ffoucart force-pushed the InverseJacobianInertialToFluid branch 2 times, most recently from f315183 to 78bb9f6 Compare February 29, 2024 00:01

wthrowe requested changes Feb 29, 2024

View reviewed changes

ffoucart force-pushed the InverseJacobianInertialToFluid branch from 78bb9f6 to 76e8d92 Compare February 29, 2024 20:57

wthrowe approved these changes Feb 29, 2024

View reviewed changes

wthrowe requested changes Mar 1, 2024

View reviewed changes

Add InverseJacobianInertialToFluidCompute

e8242e3

ffoucart force-pushed the InverseJacobianInertialToFluid branch from 76e8d92 to e8242e3 Compare March 1, 2024 19:28

ffoucart mentioned this pull request Mar 2, 2024

Calculate packet energy for Monte-Carlo #5821

Merged

3 tasks

wthrowe approved these changes Mar 5, 2024

View reviewed changes

wthrowe merged commit 477c3b2 into sxs-collaboration:develop Mar 5, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add InverseJacobianInertialToFluidCompute #5811

Add InverseJacobianInertialToFluidCompute #5811

ffoucart commented Feb 27, 2024 •

edited by nilsvu

Loading

ffoucart commented Feb 28, 2024 •

edited

Loading

wthrowe Feb 28, 2024

wthrowe Feb 28, 2024

wthrowe Feb 28, 2024

wthrowe Feb 28, 2024

wthrowe left a comment

wthrowe Feb 29, 2024

ffoucart commented Feb 29, 2024

wthrowe left a comment

ffoucart commented Mar 1, 2024

wthrowe commented Mar 1, 2024

ffoucart commented Mar 1, 2024

wthrowe left a comment

Add InverseJacobianInertialToFluidCompute #5811

Add InverseJacobianInertialToFluidCompute #5811

Conversation

ffoucart commented Feb 27, 2024 • edited by nilsvu Loading

Proposed changes

Upgrade instructions

Code review checklist

Further comments

ffoucart commented Feb 28, 2024 • edited Loading

wthrowe Feb 28, 2024

Choose a reason for hiding this comment

wthrowe Feb 28, 2024

Choose a reason for hiding this comment

wthrowe Feb 28, 2024

Choose a reason for hiding this comment

wthrowe Feb 28, 2024

Choose a reason for hiding this comment

wthrowe left a comment

Choose a reason for hiding this comment

wthrowe Feb 29, 2024

Choose a reason for hiding this comment

ffoucart commented Feb 29, 2024

wthrowe left a comment

Choose a reason for hiding this comment

ffoucart commented Mar 1, 2024

wthrowe commented Mar 1, 2024

ffoucart commented Mar 1, 2024

wthrowe left a comment

Choose a reason for hiding this comment

ffoucart commented Feb 27, 2024 •

edited by nilsvu

Loading

ffoucart commented Feb 28, 2024 •

edited

Loading