Update Newtonian Euler interface #1390

carmaza · 2019-02-24T04:47:40Z

Proposed changes

Types of changes:

Bugfix
New feature

Component:

Code
Documentation
Build system
Continuous integration

Code review checklist

The PR passes all checks, including unit tests, clang-tidy and IWYU.
For instructions on how to perform the CI checks locally refer to the Dev
guide on the Travis CI.
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.

Further comments

nilsdeppe · 2019-02-28T02:11:15Z

Could you please add descriptions to your PRs in the future that summarize the changes?

nilsdeppe

A few comments, nothing major. I'm happy with you squashing immediately

src/Evolution/Systems/NewtonianEuler/ConservativeFromPrimitive.cpp

src/Evolution/Systems/NewtonianEuler/Characteristics.hpp

src/Evolution/Systems/NewtonianEuler/SoundSpeedSquared.hpp

nilsdeppe · 2019-02-28T02:21:58Z

src/Evolution/Systems/NewtonianEuler/SoundSpeedSquared.hpp

+Scalar<DataType> sound_speed_squared(
+    const Scalar<DataType>& mass_density,
+    const Scalar<DataType>& specific_internal_energy,
+    const EquationOfStateType& equation_of_state) noexcept;


It looks like you couldn't get this to work with the base class. Could you file an issue to get this resolved in the future? I don't want to block progress on the executable for that, though.

Maybe you and I can talk about this tomorrow. I think one way to get around the templating issue would be to make the function take the base class and have the compute item function be a template for a generic EoS, then upcast to the base class. This'll mean you don't need an instantiation for each EoS while also not needing to put the function in a header.

src/Evolution/Systems/NewtonianEuler/Characteristics.hpp

mar-celine · 2019-02-28T05:38:03Z

src/Evolution/Systems/NewtonianEuler/PrimitiveFromConservative.cpp

@@ -8,66 +8,39 @@
 #include "DataStructures/Tensor/Tensor.hpp"
 #include "Utilities/GenerateInstantiations.hpp"
 #include "Utilities/Gsl.hpp"
-#include "Utilities/MakeWithValue.hpp"
+
+// IWYU pragma: no_include <array>


Need to add a blank line between no_include pragmas and no_forward_declare pragmas as in #1389

You can squash in my fix immediately as well 👍

carmaza · 2019-03-02T22:37:12Z

Squashed and rebased on develop.

nilsdeppe · 2019-03-07T03:58:47Z

src/Evolution/Systems/NewtonianEuler/Characteristics.hpp

+// IWYU pragma: no_forward_declare EquationsOfState::EquationOfState
+// IWYU pragma: no_forward_declare NewtonianEuler::Tags::MassDensity
+// IWYU pragma: no_forward_declare NewtonianEuler::Tags::Velocity
+// IWYU pragma: no_forward_declare NewtonianEuler::Tags::SoundSpeed


We're probably not consistent about this in the code, but could you alphabetize these?

nilsdeppe · 2019-03-07T04:01:27Z

src/Evolution/Systems/NewtonianEuler/SoundSpeedSquared.hpp

+Scalar<DataType> sound_speed_squared(
+    const Scalar<DataType>& mass_density,
+    const Scalar<DataType>& specific_internal_energy,
+    const EquationOfStateType& equation_of_state) noexcept;


Maybe you and I can talk about this tomorrow. I think one way to get around the templating issue would be to make the function take the base class and have the compute item function be a template for a generic EoS, then upcast to the base class. This'll mean you don't need an instantiation for each EoS while also not needing to put the function in a header.

nilsdeppe

I'm happy for you to squash the last minor comments.

@gsb76 could you please review?

- Change routine from a free function to a struct with apply - Remove `double` support (now only supports DataVector)

- Change routine from a free function to a struct with apply - Second argument is now sound speed

kidder · 2019-03-11T21:55:50Z

Current status:

@gsb76 needs to review

mar-celine

You can squash in my changes 👍

mar-celine · 2019-03-11T22:10:05Z

src/Evolution/Systems/NewtonianEuler/ConservativeFromPrimitive.hpp


+  static void apply(
+      gsl::not_null<tnsr::I<DataVector, Dim>*> momentum_density,


Should these be const?

Good question! No, because this is a declaration, not definition

mar-celine · 2019-03-11T22:12:09Z

src/Evolution/Systems/NewtonianEuler/PrimitiveFromConservative.hpp

+                                   Tags::MomentumDensity<DataVector, Dim>,
+                                   Tags::EnergyDensity<DataVector>>;
+
+  static void apply(gsl::not_null<tnsr::I<DataVector, Dim>*> velocity,


mar-celine · 2019-03-11T22:13:54Z

src/Evolution/Systems/NewtonianEuler/Characteristics.hpp

@@ -33,9 +52,37 @@ namespace NewtonianEuler {
 * and \f$c_s\f$ is the sound speed.
 */
 template <size_t Dim>
+void characteristic_speeds(
+    gsl::not_null<std::array<DataVector, Dim + 2>*> char_speeds,


Good question! No, because this is a declaration, not definition

mar-celine · 2019-03-11T22:15:31Z

tests/Unit/Evolution/Systems/NewtonianEuler/Test_Characteristics.cpp

@@ -26,10 +26,13 @@ namespace {

 template <size_t Dim>
 void test_characteristic_speeds(const DataVector& used_for_size) noexcept {


Should this be marked noexcept? I think we've been leaving out noexcept for things that call pypp.

We've been inconsistent in tests whether or not we mark things noexcept. We agreed long ago that it's not required in testing code but also not wrong to do so. Another good question!

nilsdeppe · 2019-03-11T22:28:00Z

@gsb76 let me know if you're happy with my responses :)

nilsdeppe requested review from nilsdeppe and mar-celine February 26, 2019 20:14

nilsdeppe requested changes Feb 28, 2019

View reviewed changes

mar-celine reviewed Feb 28, 2019

View reviewed changes

carmaza force-pushed the ne_update_equations branch 2 times, most recently from 0a49dd3 to 79db247 Compare March 2, 2019 22:36

carmaza added the updated all comments addressed, ready for re-review label Mar 2, 2019

carmaza force-pushed the ne_update_equations branch from 79db247 to 280363a Compare March 4, 2019 16:25

nilsdeppe reviewed Mar 7, 2019

View reviewed changes

carmaza added 3 commits March 7, 2019 11:57

Update cons from prim for Newtonian Euler

1ce33db

- Change routine from a free function to a struct with apply - Remove `double` support (now only supports DataVector)

Update prim from cons for Newtonian Euler

6b5f147

- Change routine from a free function to a struct with apply - Remove `double` support (now only supports DataVector)

Add thermodynamic_dim and is_relativistic to eos

0403ebe

carmaza force-pushed the ne_update_equations branch 3 times, most recently from e4c9a64 to 106cf33 Compare March 7, 2019 20:57

carmaza added 3 commits March 7, 2019 16:53

Add sound speed squared compute item for Newtonian Euler

7088be5

Update char speeds for Newtonian Euler

fd746e5

- Change routine from a free function to a struct with apply - Second argument is now sound speed

Add compute largest char speed for Newtonian Euler

151fb6d

carmaza force-pushed the ne_update_equations branch from 106cf33 to 151fb6d Compare March 7, 2019 21:54

mar-celine reviewed Mar 11, 2019

View reviewed changes

nilsdeppe approved these changes Mar 11, 2019

View reviewed changes

mar-celine approved these changes Mar 11, 2019

View reviewed changes

nilsdeppe merged commit aaf5e0c into sxs-collaboration:develop Mar 11, 2019

carmaza deleted the ne_update_equations branch April 1, 2019 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Newtonian Euler interface #1390

Update Newtonian Euler interface #1390

carmaza commented Feb 24, 2019 •

edited by nilsdeppe

nilsdeppe commented Feb 28, 2019

nilsdeppe left a comment

nilsdeppe Feb 28, 2019

nilsdeppe Mar 7, 2019

mar-celine Feb 28, 2019

mar-celine Feb 28, 2019

carmaza commented Mar 2, 2019

nilsdeppe Mar 7, 2019

nilsdeppe Mar 7, 2019

nilsdeppe left a comment

kidder commented Mar 11, 2019 •

edited by mar-celine

mar-celine left a comment

mar-celine Mar 11, 2019

nilsdeppe Mar 11, 2019

mar-celine Mar 11, 2019

mar-celine Mar 11, 2019

nilsdeppe Mar 11, 2019

mar-celine Mar 11, 2019

nilsdeppe Mar 11, 2019

nilsdeppe commented Mar 11, 2019


		static void apply(
		gsl::not_null<tnsr::I<DataVector, Dim>*> momentum_density,

		@@ -26,10 +26,13 @@ namespace {

		template <size_t Dim>
		void test_characteristic_speeds(const DataVector& used_for_size) noexcept {

Update Newtonian Euler interface #1390

Update Newtonian Euler interface #1390

Conversation

carmaza commented Feb 24, 2019 • edited by nilsdeppe

Proposed changes

Types of changes:

Component:

Code review checklist

Further comments

nilsdeppe commented Feb 28, 2019

nilsdeppe 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

carmaza commented Mar 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsdeppe left a comment

Choose a reason for hiding this comment

kidder commented Mar 11, 2019 • edited by mar-celine

mar-celine 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

nilsdeppe commented Mar 11, 2019

carmaza commented Feb 24, 2019 •

edited by nilsdeppe

kidder commented Mar 11, 2019 •

edited by mar-celine