Add self-start procedure to CCE #3304

moxcodes · 2021-06-18T23:36:23Z

Proposed changes

Previously, CCE did not participate in self-start. We seem to have 'gotten away' with it in cases of multistep methods on the rare standalone run that used it, but it's definitely not the right way to do things.
This makes a few tweaks to the self start procedure necessary to support CCE, and includes the self start procedure in the CCE actions.

Upgrade instructions

Any changes to the CCE initialization procedure involving the time step will likely need to be rethought in light of these changes.

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
major new feature if appropriate.

wthrowe · 2021-06-21T18:16:43Z

src/Time/Actions/SelfStartActions.hpp

+
+template <typename TagList>
+struct get_all_history_tags {
+  using type =


Is there a reason for this to be a lazy metafunction rather than an eager one?

wthrowe · 2021-06-21T18:23:40Z

tests/Unit/Time/Actions/Test_SelfStartActions.cpp

 // Returns true if the last action jumped.
-template <typename Stop, typename Whitelist, bool HasPrimitives>
+template <typename Stop, typename Whitelist, bool MultipleHistories = false,
+          bool HasPrimitives>


Default isn't useful here. If HasPrimitives can be deduced, then so can MultipleHistories.

wthrowe · 2021-06-21T18:27:14Z

src/Utilities/TypeTraits/IsA.hpp

 /// \see is_a
 template <template <typename...> class U, typename... Args>
 using is_a_t = typename is_a<U, Args...>::type;
 /// @}


Typo in commit message: "tt::is_a lambda" -> "tt::is_a_lambda"

wthrowe · 2021-06-21T18:35:41Z

tests/Unit/Evolution/Systems/Cce/Actions/Test_AnalyticBoundaryCommunication.cpp

-          l_max, end_time, start_time, number_of_radial_points,
-          std::make_unique<::TimeSteppers::DormandPrince5>()}};
+          l_max, end_time, start_time,
+          std::make_unique<::TimeSteppers::DormandPrince5>(),
+          number_of_radial_points}};


For my understanding, this is because of the change in InitializeWorldtubeBoundary::const_global_cache_tags, correct?

Yes, that's right -- in this case, the runtime system needed the time stepper regardless, but adding it to InitializeWorldtubeBoundary::const_global_cache_tags reordered the tag list.

wthrowe · 2021-06-21T18:41:02Z

src/Evolution/Systems/Cce/Actions/InitializeFirstHypersurface.hpp

+        db::get<::Tags::TimeStepId>(box)
+                .substep_time()
+                .fraction()
+                .numerator() != 0) {


I think just .fraction() != 0 is clearer.

wthrowe · 2021-06-21T18:44:32Z

src/Evolution/Systems/Cce/AnalyticBoundaryDataManager.hpp


  size_t get_l_max() const noexcept { return l_max_; }

+  const std::unique_ptr<Solutions::WorldtubeData>& get_generator()


Do you actually need the unique_ptr, or can you just return a reference to the contained value?

Also, I think this should be in the "Allow non-monotonic steps in RT analytic solution for self-start" commit.

moxcodes · 2021-06-22T20:08:48Z

fixups posted.
Cheers!

wthrowe · 2021-06-23T17:56:16Z

Fixups look fine, but apparently something went wrong because some configurations are segfaulting now.

Sometimes (particularly in CCE), we need to store multiple histories because the underlying variable types differ. This change permits using multiple histories during self-start, under the constraint that the integration order of the histories must match.

moxcodes · 2021-06-23T21:52:28Z

fixups squashed in, along with a fix for the segfault.
The mistake was I used the & operator on the returned lvalue ref -- I need to use std::addressof for that. The diff for that change (applied to Allow non-monotonic... commit) is:

--- a/src/Evolution/Systems/Cce/Actions/InitializeWorldtubeBoundary.hpp
+++ b/src/Evolution/Systems/Cce/Actions/InitializeWorldtubeBoundary.hpp
@@ -60,9 +60,9 @@ struct InitializeWorldtubeBoundaryBase {
                     const ParallelComponent* const /*meta*/) noexcept {
     if constexpr (std::is_same_v<Tags::AnalyticBoundaryDataManager,
                                  tmpl::front<ManagerTags>>) {
-      if (dynamic_cast<const Solutions::RobinsonTrautman*>(
-              &(db::get<Tags::AnalyticBoundaryDataManager>(box)
-                    .get_generator())) != nullptr) {
+      if (dynamic_cast<const Solutions::RobinsonTrautman*>(std::addressof(
+              (db::get<Tags::AnalyticBoundaryDataManager>(box)
+               .get_generator()))) != nullptr) {
         if(db::get<::Tags::TimeStepper<>>(box).number_of_substeps() != 1) {
           ERROR(
               "Do not use RobinsonTrautman analytic solution with a "

Cheers!

wthrowe · 2021-06-25T01:37:46Z

Why is that different here?

moxcodes · 2021-06-25T18:12:27Z

hm, you're right, the edge case was more peculiar. I don't properly understand the failure, because the case that caused a segfault in the dynamic_cast, when printed out, both of the versions were null pointers. However, it was my understanding that dynamic_cast was supposed to work fine with a null pointer (just returning a null pointer as well). And why std::addressof should work differently from & here is also mysterious now that I look into it further...
The better fix was updating the test to actually construct the object with a generator so the dynamic cast is actually applied to a pointer to a polymorphic type:

diff --git a/src/Evolution/Systems/Cce/Actions/InitializeWorldtubeBoundary.hpp b/src/Evolution/Systems/Cce/Actions/InitializeWorldtubeBoundary.hpp
index db638051a0..761b6f4ba3 100644
--- a/src/Evolution/Systems/Cce/Actions/InitializeWorldtubeBoundary.hpp
+++ b/src/Evolution/Systems/Cce/Actions/InitializeWorldtubeBoundary.hpp
@@ -61,8 +61,8 @@ struct InitializeWorldtubeBoundaryBase {
     if constexpr (std::is_same_v<Tags::AnalyticBoundaryDataManager,
                                  tmpl::front<ManagerTags>>) {
       if (dynamic_cast<const Solutions::RobinsonTrautman*>(
-              std::addressof((db::get<Tags::AnalyticBoundaryDataManager>(box)
-                                  .get_generator()))) != nullptr) {
+              &(db::get<Tags::AnalyticBoundaryDataManager>(box)
+                    .get_generator())) != nullptr) {
         if(db::get<::Tags::TimeStepper<>>(box).number_of_substeps() != 1) {
           ERROR(
               "Do not use RobinsonTrautman analytic solution with a "
diff --git a/tests/Unit/Evolution/Systems/Cce/Actions/Test_InitializeWorldtubeBoundary.cpp b/tests/Unit/Evolution/Systems/Cce/Actions/Test_InitializeWorldtubeBoundary.cpp
index 1f593c7eab..634af40ac8 100644
--- a/tests/Unit/Evolution/Systems/Cce/Actions/Test_InitializeWorldtubeBoundary.cpp
+++ b/tests/Unit/Evolution/Systems/Cce/Actions/Test_InitializeWorldtubeBoundary.cpp
@@ -14,6 +14,7 @@
 #include "Domain/Structure/ElementId.hpp"
 #include "Evolution/Systems/Cce/Actions/InitializeWorldtubeBoundary.hpp"
 #include "Evolution/Systems/Cce/AnalyticSolutions/RobinsonTrautman.hpp"
+#include "Evolution/Systems/Cce/AnalyticSolutions/RotatingSchwarzschild.hpp"
 #include "Evolution/Systems/Cce/BoundaryData.hpp"
 #include "Evolution/Systems/Cce/Components/WorldtubeBoundary.hpp"
 #include "Evolution/Systems/Cce/InterfaceManagers/GhLockstep.hpp"
@@ -209,8 +210,10 @@ void test_analytic_initialization() noexcept {
       {l_max, 100.0, 0.0, std::make_unique<::TimeSteppers::RungeKutta3>()}};
 
   runner.set_phase(AnalyticMetavariables::Phase::Initialization);
-  ActionTesting::emplace_component<component>(&runner, 0,
-                                              AnalyticBoundaryDataManager{});
+  ActionTesting::emplace_component<component>(
+      &runner, 0,
+      AnalyticBoundaryDataManager{
+          12_st, 20.0, std::make_unique<Solutions::RotatingSchwarzschild>()});
   // this should run the initialization
   for (size_t i = 0; i < 3; ++i) {
     ActionTesting::next_action<component>(make_not_null(&runner), 0);

I also added a commit "Include ExpectedOutput in CCE Robinson-Trautman test" to fix a test failure that appeared on the most recent CI run.

Cheers!

moxcodes · 2021-06-25T22:13:25Z

a test failed again, this time with a timeout on the RT test case, so I tweaked the parameters to make it run a bit more quickly. I guess now that it does self-start, it's a little slower because it needs to take more steps.
Cheers!

wthrowe

I don't see anything wrong with either version of the code, so I'm going to call it a compiler bug and not worry about it.

nilsdeppe

LGTM, just one question. If you decide to change anything, please squash immediately :)

nilsdeppe · 2021-06-28T21:30:33Z

src/Evolution/Systems/Cce/Actions/InitializeWorldtubeBoundary.hpp

+          ERROR(
+              "Do not use RobinsonTrautman analytic solution with a "
+              "substep-based timestepper. This is to prevent severe slowdowns "
+              "in the current RobinsonTrautman implementation");


Is what these slowdowns are documented somewhere? If not, would it be useful to list them in the error message here so that if someone really wants to use a substep integrator they can?

But also prohibit using substep methods, because that will cause performance problems

- tweak parameters to make run faster for fewer timeouts - include `ExpectedOutput` so directories are cleaned up before re-running the test

moxcodes · 2021-06-29T00:00:30Z

Added an additional note to the RobinsonTrautman solution better explaining the trouble with substep methods, and referred to that documentation in the error message.
Cheers!

wthrowe requested changes Jun 21, 2021

View reviewed changes

moxcodes force-pushed the cce_self_start branch from 3e4862d to 3f84ffa Compare June 22, 2021 20:08

moxcodes added 3 commits June 23, 2021 14:47

Consistently use System template parameter in SelfStart actions

e34072c

Allow template template packs in tt::is_a_lambda

da592b8

moxcodes force-pushed the cce_self_start branch from 3f84ffa to 27a95d8 Compare June 23, 2021 21:50

moxcodes force-pushed the cce_self_start branch from 27a95d8 to 04bb714 Compare June 25, 2021 18:07

moxcodes added a commit to moxcodes/spectre that referenced this pull request Jun 25, 2021

PR sxs-collaboration#3304

fc61f8e

moxcodes force-pushed the cce_self_start branch from 04bb714 to 5cdcd5e Compare June 25, 2021 22:11

moxcodes added a commit to moxcodes/spectre that referenced this pull request Jun 26, 2021

PR sxs-collaboration#3304

f5d9bb4

moxcodes added a commit to moxcodes/spectre that referenced this pull request Jun 26, 2021

PR sxs-collaboration#3304

8deba0f

moxcodes mentioned this pull request Jun 28, 2021

Add local time-stepping for CCE #3326

Merged

4 tasks

wthrowe previously approved these changes Jun 28, 2021

View reviewed changes

nilsdeppe previously approved these changes Jun 28, 2021

View reviewed changes

moxcodes dismissed stale reviews from nilsdeppe and wthrowe via 69c123c June 28, 2021 23:58

moxcodes force-pushed the cce_self_start branch from 5cdcd5e to 69c123c Compare June 28, 2021 23:58

moxcodes added 3 commits June 28, 2021 16:59

Allow non-monotonic steps in RT analytic solution for self-start

e9300e4

But also prohibit using substep methods, because that will cause performance problems

Add self-start to CCE

727b496

Fix occasional failures in RobinsonTrautman test

52ea7de

- tweak parameters to make run faster for fewer timeouts - include `ExpectedOutput` so directories are cleaned up before re-running the test

moxcodes force-pushed the cce_self_start branch from 69c123c to 52ea7de Compare June 28, 2021 23:59

nilsdeppe approved these changes Jun 29, 2021

View reviewed changes

moxcodes added a commit to moxcodes/spectre that referenced this pull request Jul 1, 2021

PR sxs-collaboration#3304

89ef0e4

moxcodes mentioned this pull request Jul 1, 2021

Add error-based step chooser to executables #3342

Merged

5 tasks

wthrowe approved these changes Jul 1, 2021

View reviewed changes

wthrowe merged commit 08973d0 into sxs-collaboration:develop Jul 1, 2021


		size_t get_l_max() const noexcept { return l_max_; }

		const std::unique_ptr<Solutions::WorldtubeData>& get_generator()

Add self-start procedure to CCE #3304

Add self-start procedure to CCE #3304

Uh oh!

Conversation

moxcodes commented Jun 18, 2021

Proposed changes

Upgrade instructions

Code review checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moxcodes commented Jun 22, 2021

Uh oh!

wthrowe commented Jun 23, 2021

Uh oh!

moxcodes commented Jun 23, 2021

Uh oh!

wthrowe commented Jun 25, 2021

Uh oh!

moxcodes commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moxcodes commented Jun 25, 2021

Uh oh!

wthrowe left a comment

Choose a reason for hiding this comment

Uh oh!

nilsdeppe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moxcodes commented Jun 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moxcodes commented Jun 25, 2021 •

edited

Loading