From 9dc6c0104ec1b42d8cb4b0107607f7183e407da7 Mon Sep 17 00:00:00 2001 From: William Throwe Date: Tue, 23 Jan 2024 16:44:05 -0500 Subject: [PATCH 1/2] Check dense output ability before postprocessors If the TimeStepper knows it cannot produce dense output without another substep, we should not wait for the preprocessors. The preprocessors can depend on the results of later substeps, which would unnecessarily deadlock. --- .../Actions/RunEventsAndDenseTriggers.hpp | 30 +++++++++---------- .../Test_RunEventsAndDenseTriggers.cpp | 4 +-- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/Evolution/Actions/RunEventsAndDenseTriggers.hpp b/src/Evolution/Actions/RunEventsAndDenseTriggers.hpp index 15a94b3b04c5..bdeec2eb5c4f 100644 --- a/src/Evolution/Actions/RunEventsAndDenseTriggers.hpp +++ b/src/Evolution/Actions/RunEventsAndDenseTriggers.hpp @@ -244,21 +244,6 @@ struct RunEventsAndDenseTriggers { case TriggeringState::NotReady: return {Parallel::AlgorithmExecution::Retry, std::nullopt}; case TriggeringState::NeedsEvolvedVariables: { - bool ready = true; - tmpl::for_each([&](auto postprocessor_v) { - using postprocessor = tmpl::type_from; - if (ready) { - if (not postprocessor::is_ready(make_not_null(&box), - make_not_null(&inboxes), cache, - array_index, component)) { - ready = false; - } - } - }); - if (not ready) { - return {Parallel::AlgorithmExecution::Retry, std::nullopt}; - } - using history_tag = ::Tags::HistoryEvolvedVariables; bool dense_output_succeeded = false; variables_restorer.save(); @@ -279,6 +264,21 @@ struct RunEventsAndDenseTriggers { return {Parallel::AlgorithmExecution::Continue, std::nullopt}; } + bool ready = true; + tmpl::for_each([&](auto postprocessor_v) { + using postprocessor = tmpl::type_from; + if (ready) { + if (not postprocessor::is_ready(make_not_null(&box), + make_not_null(&inboxes), cache, + array_index, component)) { + ready = false; + } + } + }); + if (not ready) { + return {Parallel::AlgorithmExecution::Retry, std::nullopt}; + } + postprocessor_restorer.save(); tmpl::for_each([&box](auto postprocessor_v) { using postprocessor = tmpl::type_from; diff --git a/tests/Unit/Evolution/Actions/Test_RunEventsAndDenseTriggers.cpp b/tests/Unit/Evolution/Actions/Test_RunEventsAndDenseTriggers.cpp index 2097b747a22d..4109b7efc412 100644 --- a/tests/Unit/Evolution/Actions/Test_RunEventsAndDenseTriggers.cpp +++ b/tests/Unit/Evolution/Actions/Test_RunEventsAndDenseTriggers.cpp @@ -516,11 +516,11 @@ void test(const bool time_runs_forward) { }, make_not_null(&box), db::get>(box)); } + CHECK(run_if_ready(make_not_null(&runner))); if (data_needed) { - TestCase::check_dense(&runner, true, {}); + TestEvent::check_calls({}); } else { // If we don't need the data, it shouldn't matter whether it is missing. - CHECK(run_if_ready(make_not_null(&runner))); TestEvent::check_calls({{step_center, stored_vars}}); } }; From 8e39367f464800645026e5ab0a6a6a92bf78fb39 Mon Sep 17 00:00:00 2001 From: William Throwe Date: Tue, 12 Mar 2024 14:56:35 -0400 Subject: [PATCH 2/2] Don't dense-output to the end of the step This will be necessary for monotonic Adams-Moulton, as the solution is discontinuous there. It should be fine for the others, although it might be nice if we passed more information so the TimeStepper could make the choice. --- .../Actions/RunEventsAndDenseTriggers.hpp | 5 +++-- src/Time/TimeSteppers/AdamsMoultonPc.cpp | 4 ---- src/Time/TimeSteppers/Rk3HesthavenSsp.cpp | 5 ----- src/Time/TimeSteppers/RungeKutta.cpp | 5 ----- .../Actions/Test_RunEventsAndDenseTriggers.cpp | 14 ++++++++++++++ .../Time/TimeSteppers/TimeStepperTestUtils.cpp | 4 ++-- 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/Evolution/Actions/RunEventsAndDenseTriggers.hpp b/src/Evolution/Actions/RunEventsAndDenseTriggers.hpp index bdeec2eb5c4f..a0175467b29c 100644 --- a/src/Evolution/Actions/RunEventsAndDenseTriggers.hpp +++ b/src/Evolution/Actions/RunEventsAndDenseTriggers.hpp @@ -204,7 +204,8 @@ struct RunEventsAndDenseTriggers { const auto step_end = time_step_id.step_time() + db::get<::Tags::TimeStep>(box); - const evolution_less before{time_step_id.time_runs_forward()}; + const evolution_less_equal before_equal{ + time_step_id.time_runs_forward()}; using postprocessor_return_tags = tmpl::join>>; @@ -223,7 +224,7 @@ struct RunEventsAndDenseTriggers { for (;;) { const double next_trigger = events_and_dense_triggers.next_trigger(box); - if (before(step_end.value(), next_trigger)) { + if (before_equal(step_end.value(), next_trigger)) { return {Parallel::AlgorithmExecution::Continue, std::nullopt}; } diff --git a/src/Time/TimeSteppers/AdamsMoultonPc.cpp b/src/Time/TimeSteppers/AdamsMoultonPc.cpp index ab14b2057f52..6d880ab68936 100644 --- a/src/Time/TimeSteppers/AdamsMoultonPc.cpp +++ b/src/Time/TimeSteppers/AdamsMoultonPc.cpp @@ -184,10 +184,6 @@ template bool AdamsMoultonPc::dense_update_u_impl(const gsl::not_null u, const ConstUntypedHistory& history, const double time) const { - // Special case required to handle the initial time. - if (time == history.back().time_step_id.step_time().value()) { - return true; - } if (history.at_step_start()) { return false; } diff --git a/src/Time/TimeSteppers/Rk3HesthavenSsp.cpp b/src/Time/TimeSteppers/Rk3HesthavenSsp.cpp index 9f746b788308..bad89b02e5e7 100644 --- a/src/Time/TimeSteppers/Rk3HesthavenSsp.cpp +++ b/src/Time/TimeSteppers/Rk3HesthavenSsp.cpp @@ -113,11 +113,6 @@ bool Rk3HesthavenSsp::dense_update_u_impl(const gsl::not_null u, } const double step_start = history.front().time_step_id.step_time().value(); const double step_end = history.back().time_step_id.step_time().value(); - if (history.size() == 1 and time == step_end) { - // Special case necessary for dense output at the initial time, - // before taking a step. - return true; - } const evolution_less before{step_end > step_start}; if (history.size() == 1 or before(step_end, time)) { return false; diff --git a/src/Time/TimeSteppers/RungeKutta.cpp b/src/Time/TimeSteppers/RungeKutta.cpp index 2658bc53f1dd..1112068a7a75 100644 --- a/src/Time/TimeSteppers/RungeKutta.cpp +++ b/src/Time/TimeSteppers/RungeKutta.cpp @@ -153,11 +153,6 @@ bool RungeKutta::dense_update_u_impl(const gsl::not_null u, } const double step_start = history.front().time_step_id.step_time().value(); const double step_end = history.back().time_step_id.step_time().value(); - if (history.size() == 1 and time == step_end) { - // Special case necessary for dense output at the initial time, - // before taking a step. - return true; - } const evolution_less before{step_end > step_start}; if (history.size() == 1 or before(step_end, time)) { return false; diff --git a/tests/Unit/Evolution/Actions/Test_RunEventsAndDenseTriggers.cpp b/tests/Unit/Evolution/Actions/Test_RunEventsAndDenseTriggers.cpp index 4109b7efc412..5b847986335b 100644 --- a/tests/Unit/Evolution/Actions/Test_RunEventsAndDenseTriggers.cpp +++ b/tests/Unit/Evolution/Actions/Test_RunEventsAndDenseTriggers.cpp @@ -434,6 +434,20 @@ void test(const bool time_runs_forward) { check_not_reached(true); check_not_reached(false); + // Triggers at the end of the step (should not run) + const auto check_step_end = [&set_up_component, &start_time, &step_size]( + const std::optional& is_triggered) { + MockRuntimeSystem runner{ + {std::make_unique(1)}}; + set_up_component( + &runner, {{start_time + step_size, is_triggered, std::nullopt, false}}); + CHECK(run_if_ready(make_not_null(&runner))); + TestEvent::check_calls({}); + }; + check_step_end(std::nullopt); + check_step_end(true); + check_step_end(false); + // Trigger isn't ready { MockRuntimeSystem runner{ diff --git a/tests/Unit/Helpers/Time/TimeSteppers/TimeStepperTestUtils.cpp b/tests/Unit/Helpers/Time/TimeSteppers/TimeStepperTestUtils.cpp index 19ef611b769d..737be7a99dd5 100644 --- a/tests/Unit/Helpers/Time/TimeSteppers/TimeStepperTestUtils.cpp +++ b/tests/Unit/Helpers/Time/TimeSteppers/TimeStepperTestUtils.cpp @@ -361,13 +361,13 @@ void check_dense_output( auto step = step_size; for (;;) { history.insert(time_id, y, y); - if (not before((time_id.step_time() + step).value(), time)) { + if (before(time, (time_id.step_time() + step).value())) { // Make sure the initial value is preserved. y = 2.0 * *history.complete_step_start().value; if (stepper.dense_update_u(make_not_null(&y), history, time)) { return y - *history.complete_step_start().value; } - REQUIRE(before(time_id.step_time().value(), time)); + REQUIRE(not before(time, time_id.step_time().value())); } y = std::numeric_limits::signaling_NaN(); if (use_error_methods) {