Skip to content
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.

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

Change TerminatePhase return value #6335

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/ParallelAlgorithms/Actions/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ spectre_target_headers(
InitializeItems.hpp
LimiterActions.hpp
MutateApply.hpp
PausePhase.hpp
RandomizeVariables.hpp
SetData.hpp
TerminatePhase.hpp
Expand Down
44 changes: 44 additions & 0 deletions src/ParallelAlgorithms/Actions/PausePhase.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Distributed under the MIT License.
// See LICENSE.txt for details.

#pragma once

#include <optional>

#include "Parallel/AlgorithmExecution.hpp"

/// \cond
namespace tuples {
template <typename... InboxTags>
struct TaggedTuple;
} // namespace tuples
namespace Parallel {
template <typename Metavariables>
class GlobalCache;
} // namespace Parallel
/// \endcond

namespace Parallel::Actions {

/*!
* \ingroup ActionsGroup
* \brief Pause the algorithm by returning
* `Parallel::AlgorithmExecution::Pause`.
*/
struct PausePhase {
template <typename DataBox, typename... InboxTags, typename Metavariables,
typename ArrayIndex, typename ActionList,
typename ParallelComponent>
static Parallel::iterable_action_return_t apply(
DataBox& /*box*/, const tuples::TaggedTuple<InboxTags...>& /*inboxes*/,
const Parallel::GlobalCache<Metavariables>& /*cache*/,
const ArrayIndex& /*array_index*/,
// NOLINTNEXTLINE(readability-avoid-const-params-in-decls)
const ActionList /*meta*/,
// NOLINTNEXTLINE(readability-avoid-const-params-in-decls)
const ParallelComponent* const /*meta*/) {
return {Parallel::AlgorithmExecution::Pause, std::nullopt};
}
};

} // namespace Parallel::Actions
11 changes: 5 additions & 6 deletions src/ParallelAlgorithms/Actions/TerminatePhase.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ class GlobalCache;
} // namespace Parallel
/// \endcond

namespace Parallel {
namespace Actions {
namespace Parallel::Actions {

/*!
* \ingroup ActionsGroup
* \brief Terminate the algorithm to proceed to the next phase.
* \brief Halt the algorithm and wait to proceed to the next phase by returning
* `Parallel::AlgorithmExecution::Halt`
*/
struct TerminatePhase {
template <typename DataBox, typename... InboxTags, typename Metavariables,
Expand All @@ -37,9 +37,8 @@ struct TerminatePhase {
const ActionList /*meta*/,
// NOLINTNEXTLINE(readability-avoid-const-params-in-decls)
const ParallelComponent* const /*meta*/) {
return {Parallel::AlgorithmExecution::Pause, std::nullopt};
return {Parallel::AlgorithmExecution::Halt, std::nullopt};
}
};

} // namespace Actions
} // namespace Parallel
} // namespace Parallel::Actions
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
#include "Parallel/Phase.hpp"
#include "Parallel/PhaseDependentActionList.hpp"
#include "ParallelAlgorithms/Actions/InitializeItems.hpp"
#include "ParallelAlgorithms/Actions/PausePhase.hpp"
#include "ParallelAlgorithms/Actions/SetData.hpp"
#include "ParallelAlgorithms/Actions/TerminatePhase.hpp"
#include "ParallelAlgorithms/Amr/Actions/Component.hpp"
Expand Down Expand Up @@ -437,16 +438,16 @@ struct ElementArray {
Dim, subdomain_init_tags, DummyOptionsGroup, false,
TemporalIdTag>,
init_subdomain_action, init_random_subdomain_data_action,
Parallel::Actions::TerminatePhase,
Parallel::Actions::PausePhase,
// Full DG operator
typename dg_operator::apply_actions,
Parallel::Actions::TerminatePhase,
Parallel::Actions::PausePhase,
// Subdomain operator
ApplySubdomainOperator<SubdomainOperator,
typename fields_tag::tags_list>,
TestSubdomainOperatorMatrix<SubdomainOperator,
typename fields_tag::tags_list>,
Parallel::Actions::TerminatePhase>>>;
Parallel::Actions::PausePhase>>>;
};

template <typename Metavariables>
Expand Down Expand Up @@ -605,8 +606,7 @@ void test_subdomain_operator(
{initial_ref_levs, initial_extents, SubdomainOperator{},
typename subdomain_operator_applied_to_fields_tag::type{},
override_boundary_conditions});
while (
not ActionTesting::get_terminate<element_array>(runner, element_id)) {
for (size_t i = 0; i < 4 + tmpl::size<ExtraInitActions>::value; i++) {
ActionTesting::next_action<element_array>(make_not_null(&runner),
element_id);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
#include "Parallel/Phase.hpp"
#include "ParallelAlgorithms/Actions/Goto.hpp"
#include "ParallelAlgorithms/Actions/InitializeItems.hpp"
#include "ParallelAlgorithms/Actions/PausePhase.hpp"
#include "ParallelAlgorithms/Actions/SetData.hpp"
#include "ParallelAlgorithms/Actions/TerminatePhase.hpp"
#include "ParallelAlgorithms/Amr/Actions/Component.hpp"
Expand Down Expand Up @@ -156,7 +157,7 @@ struct ElementArray {
System, fixed_sources_tag>,
::Actions::Label<ApplyOperatorStart>,
typename dg_operator::apply_actions, IncrementTemporalId,
Parallel::Actions::TerminatePhase>>>;
Parallel::Actions::PausePhase>>>;
};

template <typename Metavariables>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,10 @@ SPECTRE_TEST_CASE(
std::make_unique<::TimeSteppers::DormandPrince5>()));

// Run the initializations
for (size_t i = 0; i < 6; ++i) {
for (size_t i = 0; i < 5; ++i) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a ton of unexplained changes like this in this PR. What is going on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 5 actions in the initialization phase for this component. Previously the last one returned Pause, so when it called ActionTesting::next_action one extra time, nothing bad happened. But now that the last action returns Halt, calling it an extra time causes an error. So it was a bug previously to call it 6 times. Same for the rest of the changes like this

ActionTesting::next_action<evolution_component>(make_not_null(&runner), 0);
}
for (size_t i = 0; i < 3; ++i) {
for (size_t i = 0; i < 2; ++i) {
ActionTesting::next_action<worldtube_component>(make_not_null(&runner), 0);
}
runner.set_phase(Parallel::Phase::Evolve);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,10 +257,10 @@ SPECTRE_TEST_CASE("Unit.Evolution.Systems.Cce.Actions.H5BoundaryCommunication",
false, false, std::optional<double>{}));

// this should run the initializations
for (size_t i = 0; i < 5; ++i) {
for (size_t i = 0; i < 4; ++i) {
ActionTesting::next_action<evolution_component>(make_not_null(&runner), 0);
}
for (size_t i = 0; i < 2; ++i) {
for (size_t i = 0; i < 1; ++i) {
ActionTesting::next_action<worldtube_component>(make_not_null(&runner), 0);
}
ActionTesting::set_phase(make_not_null(&runner), Parallel::Phase::Evolve);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ SPECTRE_TEST_CASE(
target_step_size, scri_plus_interpolation_order);

// this should run the initialization
for (size_t i = 0; i < 6; ++i) {
for (size_t i = 0; i < 5; ++i) {
ActionTesting::next_action<component>(make_not_null(&runner), 0);
}
ActionTesting::set_phase(make_not_null(&runner), Parallel::Phase::Evolve);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ void test_klein_gordon_cce_initialization(const gsl::not_null<Generator*> gen) {
target_step_size, scri_plus_interpolation_order);

// go through the action list
for (size_t i = 0; i < 7; ++i) {
for (size_t i = 0; i < 6; ++i) {
ActionTesting::next_action<component>(make_not_null(&runner), 0);
}
ActionTesting::set_phase(make_not_null(&runner), Parallel::Phase::Evolve);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ void test_h5_initialization(const gsl::not_null<Generator*> gen) {
false, false, std::optional<double>{}));

// this should run the initialization
for (size_t i = 0; i < 3; ++i) {
for (size_t i = 0; i < 2; ++i) {
ActionTesting::next_action<component>(make_not_null(&runner), 0);
}
ActionTesting::set_phase(make_not_null(&runner), Parallel::Phase::Evolve);
Expand Down Expand Up @@ -177,7 +177,7 @@ void test_gh_initialization() {
InterfaceManagers::GhLockstep{});

// this should run the initialization
for (size_t i = 0; i < 3; ++i) {
for (size_t i = 0; i < 2; ++i) {
ActionTesting::next_action<component>(make_not_null(&runner), 0);
}
runner.set_phase(Parallel::Phase::Evolve);
Expand Down Expand Up @@ -210,7 +210,7 @@ void test_analytic_initialization() {
static_cast<std::unique_ptr<TimeStepper>>(
std::make_unique<::TimeSteppers::Rk3HesthavenSsp>()));
// this should run the initialization
for (size_t i = 0; i < 3; ++i) {
for (size_t i = 0; i < 2; ++i) {
ActionTesting::next_action<component>(make_not_null(&runner), 0);
}
runner.set_phase(Parallel::Phase::Evolve);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ SPECTRE_TEST_CASE(
ActionTesting::emplace_component<MockObserver<test_metavariables>>(&runner,
0);
// the initialization actions
for (size_t i = 0; i < 6; ++i) {
for (size_t i = 0; i < 5; ++i) {
ActionTesting::next_action<evolution_component>(make_not_null(&runner), 0);
}
runner.set_phase(Parallel::Phase::Evolve);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,10 +236,10 @@ void test_klein_gordon_h5_boundary_communication(
std::optional<double>{}));

// this should run the initializations
for (size_t i = 0; i < 6; ++i) {
for (size_t i = 0; i < 5; ++i) {
ActionTesting::next_action<evolution_component>(make_not_null(&runner), 0);
}
for (size_t i = 0; i < 2; ++i) {
for (size_t i = 0; i < 1; ++i) {
ActionTesting::next_action<worldtube_component>(make_not_null(&runner), 0);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,10 +222,10 @@ SPECTRE_TEST_CASE("Unit.Evolution.Systems.Cce.Actions.RequestBoundaryData",
false, false, std::optional<double>{}));

// this should run the initializations
for (size_t i = 0; i < 5; ++i) {
for (size_t i = 0; i < 4; ++i) {
ActionTesting::next_action<evolution_component>(make_not_null(&runner), 0);
}
for(size_t i = 0; i < 3; ++i) {
for (size_t i = 0; i < 2; ++i) {
ActionTesting::next_action<worldtube_component>(make_not_null(&runner), 0);
}
ActionTesting::set_phase(make_not_null(&runner), Parallel::Phase::Evolve);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ void test_klein_gordon_boundary_data(const gsl::not_null<Generator*> gen) {
std::optional<double>{}));

// this should run the initializations
for (size_t i = 0; i < 6; ++i) {
for (size_t i = 0; i < 5; ++i) {
ActionTesting::next_action<evolution_component>(make_not_null(&runner), 0);
}
for (size_t i = 0; i < 2; ++i) {
Expand Down
1 change: 1 addition & 0 deletions tests/Unit/ParallelAlgorithms/Actions/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ set(LIBRARY_SOURCES
Test_Goto.cpp
Test_InitializeItems.cpp
Test_MutateApply.cpp
Test_PausePhase.cpp
Test_RandomizeVariables.cpp
Test_SetData.cpp
Test_TerminatePhase.cpp
Expand Down
38 changes: 38 additions & 0 deletions tests/Unit/ParallelAlgorithms/Actions/Test_PausePhase.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Distributed under the MIT License.
// See LICENSE.txt for details.

#include "Framework/TestingFramework.hpp"

#include "Framework/ActionTesting.hpp"
#include "Parallel/Phase.hpp"
#include "ParallelAlgorithms/Actions/PausePhase.hpp"
#include "Utilities/TMPL.hpp"

namespace {
template <typename Metavariables>
struct Component {
using metavariables = Metavariables;
using chare_type = ActionTesting::MockArrayChare;
using array_index = int;
using phase_dependent_action_list = tmpl::list<Parallel::PhaseActions<
Parallel::Phase::Testing, tmpl::list<Parallel::Actions::PausePhase>>>;
};

struct Metavariables {
using component_list = tmpl::list<Component<Metavariables>>;
};
} // namespace

SPECTRE_TEST_CASE("Unit.Parallel.Actions.PausePhase",
"[Unit][Parallel][Actions]") {
using component = Component<Metavariables>;

ActionTesting::MockRuntimeSystem<Metavariables> runner{{}};
ActionTesting::emplace_component<component>(&runner, 0);

ActionTesting::set_phase(make_not_null(&runner), Parallel::Phase::Testing);

CHECK_FALSE(ActionTesting::get_terminate<component>(runner, 0));
ActionTesting::next_action<component>(make_not_null(&runner), 0);
CHECK(ActionTesting::get_terminate<component>(runner, 0));
}
15 changes: 11 additions & 4 deletions tests/Unit/Time/Actions/Test_SelfStartActions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include "Parallel/AlgorithmExecution.hpp"
#include "Parallel/Phase.hpp"
#include "Parallel/PhaseDependentActionList.hpp"
#include "ParallelAlgorithms/Actions/PausePhase.hpp"
#include "ParallelAlgorithms/Actions/TerminatePhase.hpp"
#include "Time/Actions/CleanHistory.hpp"
#include "Time/Actions/RecordTimeStepperData.hpp"
#include "Time/Actions/SelfStartActions.hpp"
Expand Down Expand Up @@ -161,10 +163,15 @@ struct Component {
Actions::CleanHistory<typename metavariables::system, false>,
tmpl::conditional_t<has_primitives, Actions::UpdatePrimitives,
tmpl::list<>>>;
using action_list = tmpl::flatten<
tmpl::list<SelfStart::self_start_procedure<
step_actions, typename metavariables::system>,
step_actions>>;
// This test doesn't operate exactly like how SelfStart would normally work in
// an executable. Instead it jumps around quite a lot. Therefore to avoid any
// issues that TerminatePhase would cause, we just replace it with PausePhase.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove it. No reason to replace it with a no-op.

using action_list = tmpl::replace<
tmpl::flatten<
tmpl::list<SelfStart::self_start_procedure<
step_actions, typename metavariables::system>,
step_actions>>,
Parallel::Actions::TerminatePhase, Parallel::Actions::PausePhase>;
using phase_dependent_action_list = tmpl::list<
Parallel::PhaseActions<Parallel::Phase::Initialization,
tmpl::list<ActionTesting::InitializeDataBox<
Expand Down
Loading