New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add an analytic data PolarMagnetizedFmDisk #5563

Merged

nilsdeppe merged 2 commits into sxs-collaboration:develop from jyoo1042:disk_PR_new

Oct 15, 2023

Contributor

jyoo1042 commented Oct 12, 2023

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 requested changes

View reviewed changes

tests/Unit/Helpers/PointwiseFunctions/AnalyticData/TestHelpers.hpp

-                  CHECK(tuples::get<tag>(single_var) ==
-                        tuples::get<tag>(vars_from_all_tags_reversed));
+                  CHECK_ITERABLE_APPROX(tuples::get<tag>(single_var),
+                                        tuples::get<tag>(vars_from_all_tags));

Member

wthrowe Oct 13, 2023

Why this change?

Contributor Author

jyoo1042 Oct 13, 2023

test_tag_retrieval was giving an error when using CHECK.

Member

wthrowe Oct 13, 2023

There are no other changes in this commit, so probably this should be in one of the later commits.
Do you understand why? It might be fine, but I don't see an obvious reason for them to differ,

Member

wthrowe Oct 13, 2023

In offline discussion we decided this change is fine squashed into a later commit. It appears to be optimization related, and this class has more interaction between fetched variables than most of the others so that's not terribly unexpected.

tests/Unit/PointwiseFunctions/AnalyticData/GrMhd/Test_PolarMagnetizedFmDisk.cpp Outdated

+                          grmhd::AnalyticData::MagnetizedFmDisk(1.3, 0.345, 6.123, 14.2,
+.065, 1.654, 0.42, 85.0, 6),
+                          domain::CoordinateMaps::SphericalTorus(2.5, 3.6, 0.9, 0.7));
+                test_move_semantics(std::move(disk), disk_copy);  //  NOLINT

Member

wthrowe Oct 13, 2023

Indicate what warning is being silenced by the NOLINT as // NOLINT(warning-name) or // NOLINTNEXTLINE(warning-name), where warning-name is from the error message and will look something like performance-move-const-arg.

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

+                test_create_from_options();
+                test_serialize();
+                test_move();
+                test_variables(std::numeric_limits<double>::signaling_NaN());

Member

wthrowe Oct 13, 2023

Also test with DataVector.

tests/Unit/PointwiseFunctions/AnalyticData/GrMhd/Test_SphericalTorus.cpp Outdated

+              namespace {
+              const std::array<double, 3> t2a(const tnsr::I<double, 3>& tens);
+              const tnsr::I<double, 3> a2t(const std::array<double, 3>& arr);

Member

wthrowe Oct 13, 2023

No need to declare these here, since they are defined immediately following.

tests/Unit/PointwiseFunctions/AnalyticData/GrMhd/Test_SphericalTorus.cpp Outdated

+                          },
+                          test_point, j, 1e-3);
+                      const auto numerical1 = numerical_derivative(

Member

wthrowe Oct 13, 2023

Use more descriptive names for these variables. Maybe numerical_jacobian and numerical_inverse_jacobian or something.

tests/Unit/PointwiseFunctions/AnalyticData/GrMhd/Test_SphericalTorus.cpp Outdated

+              const std::array<double, 3> t2a(const tnsr::I<double, 3>& tens);
+              const tnsr::I<double, 3> a2t(const std::array<double, 3>& arr);
+              const std::array<double, 3> t2a(const tnsr::I<double, 3>& tens) {

Member

wthrowe Oct 13, 2023

#include "DataStructures/Tensor/Tensor.hpp"

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

+              const std::array<double, 3> t2a(const tnsr::I<double, 3>& tens) {
+                std::array<double, 3> arr{};
+                for (size_t i = 0; i < 3; i++) {
+                  gsl::at(arr,i) = tens.get(i);

Member

wthrowe Oct 13, 2023

#include "Utilities/Gsl.hpp"

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

+              const std::array<double, 3> t2a(const tnsr::I<double, 3>& tens) {
+                std::array<double, 3> arr{};
+                for (size_t i = 0; i < 3; i++) {

Member

wthrowe Oct 13, 2023

#include <cstddef>

tests/Unit/PointwiseFunctions/AnalyticData/GrMhd/Test_SphericalTorus.cpp Outdated

+                return arr;
+              }
+              const tnsr::I<double, 3> a2t(const std::array<double, 3>& arr) {

Member

wthrowe Oct 13, 2023

Remove const from return type. No reason to restrict what the caller can do with the temporary. Also on t2a.

src/Evolution/Executables/GrMhd/ValenciaDivClean/CMakeLists.txt Outdated

+              add_grmhd_executable(
+                PolarMagnetizedFmDisk
+                grmhd::AnalyticData::PolarMagnetizedFmDisk
+                "${LIBS_TO_LINK}"

Member

wthrowe Oct 13, 2023

Remove ${LIBS_TO_LINK} in this commit instead of the next one.

jyoo1042 force-pushed the disk_PR_new branch 2 times, most recently from 0aa1e9f to a4e454e Compare

October 13, 2023 21:00

wthrowe previously approved these changes

View reviewed changes

Member

wthrowe left a comment

I'm happy with this, but since I was involved in writing some of the code I shouldn't be the sole reviewer. @nilsdeppe, I think you were willing to do the follow-up?

Member

nilsdeppe commented Oct 13, 2023

Yep! I might not have time until later next week, but I'm happy to take a look, hopefully sooner :D

nilsdeppe requested changes

View reviewed changes

Member

nilsdeppe left a comment

Couple minor things, please squash right away

src/Domain/CoordinateMaps/CoordinateMap.tpp Outdated

                       std::string, std::unique_ptr<domain::FunctionsOfTime::FunctionOfTime>>&
                       functions_of_time) const {
+              (void) functions_of_time;

Member

nilsdeppe Oct 14, 2023

What do you have against [[maybe_unused]]?

Contributor Author

jyoo1042 Oct 14, 2023

I got a compiler warning even with [[maybe_unused]].
But, I can undo this change because PolarMagnetizedFmDisk.cpp no longer uses this file.

Member

nilsdeppe Oct 14, 2023

Yes, please undo since it's not needed

src/PointwiseFunctions/AnalyticData/GrMhd/PolarMagnetizedFmDisk.hpp Outdated

+              namespace grmhd::AnalyticData {
+              /*!
+               * \brief Magnetized fluid disk orbiting a Kerr black hole in the usual

Member

nilsdeppe Oct 14, 2023

maybe clarify what "usual" means?

jyoo1042 dismissed wthrowe’s stale review via

October 14, 2023 15:35

jyoo1042 force-pushed the disk_PR_new branch from a4e454e to 6746128 Compare

October 14, 2023 15:35

nilsdeppe requested changes

View reviewed changes

src/PointwiseFunctions/AnalyticData/GrMhd/PolarMagnetizedFmDisk.hpp

@@ @@ -34,9 +33,9 @@ class er; @@
               namespace grmhd::AnalyticData {
               /*!
-               * \brief Magnetized fluid disk orbiting a Kerr black hole in the usual
+               * \brief Magnetized fluid disk orbiting a Kerr black hole in the Kerr-Schild

Member

nilsdeppe Oct 14, 2023

This should go in the first commit.

src/Domain/CoordinateMaps/CoordinateMap.tpp Outdated

                       std::string, std::unique_ptr<domain::FunctionsOfTime::FunctionOfTime>>&
                       functions_of_time) const {
+              (void) functions_of_time;

Member

nilsdeppe Oct 14, 2023

Yes, please undo since it's not needed

jyoo1042 added 2 commits

October 14, 2023 12:29


          Add an analytic data PolarMagnetizedFmDisk

60c8083


          Move SphericalTorus to PointwiseFunctions/AnalyticData/GrMhd

03ea2ea

jyoo1042 force-pushed the disk_PR_new branch from 6746128 to 03ea2ea Compare

October 14, 2023 19:37

nilsdeppe approved these changes

View reviewed changes

nilsdeppe merged commit da248e2 into sxs-collaboration:develop

jyoo1042 deleted the disk_PR_new branch

October 16, 2023 13:26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet