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

Executable for MC: initialize background #6355

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

ffoucart
Copy link
Contributor

Proposed changes

Create base MC executable

Upgrade instructions

None so far

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.

@ffoucart
Copy link
Contributor Author

Work in progress -- first commit creates the background fluid and metric variables, EoS and neutrino table.

@ffoucart ffoucart added the in progress Don't review, used for sharing code and getting feedback label Oct 30, 2024
@ffoucart
Copy link
Contributor Author

@nilsdeppe Not sure if we want one big PR for the MC code, or to commit partial work once we have something useful. I'm keeping the work we have done so far (slightly cleaned up for compatibility with other executables) here.

@ffoucart ffoucart force-pushed the MCExecutable branch 2 times, most recently from 41053cd to f533cb9 Compare November 21, 2024 21:11
@ffoucart ffoucart removed the in progress Don't review, used for sharing code and getting feedback label Dec 5, 2024
@ffoucart ffoucart marked this pull request as ready for review December 5, 2024 14:30
@ffoucart ffoucart force-pushed the MCExecutable branch 3 times, most recently from 45d1b93 to 84ed5be Compare December 6, 2024 16:01
@ffoucart
Copy link
Contributor Author

ffoucart commented Dec 6, 2024

Rebased on develop to fix minor conflict with RunEventsAndTriggers (template change, option renamed)

@ffoucart ffoucart force-pushed the MCExecutable branch 2 times, most recently from eb41b51 to 9a144ff Compare December 6, 2024 19:46
This was referenced Dec 10, 2024
@ffoucart
Copy link
Contributor Author

Rebased to make dependent on #6424 . The first commit is now from that PR, the second commit creates the executable; the final part to make it work with input files is in #6403

@ffoucart
Copy link
Contributor Author

Rebased on develop after merging #6424

using initialization_actions = tmpl::list<
Initialization::Actions::InitializeItems<
Initialization::TimeStepping<EvolutionMetavars, TimeStepperBase>,
evolution::dg::Initialization::Domain<volume_dim> //,
Copy link
Member

Choose a reason for hiding this comment

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

remove the comment at the end of the line

Parallel::PhaseActions<
Parallel::Phase::Evolve,
tmpl::list<
Actions::AdvanceTime,
Copy link
Member

Choose a reason for hiding this comment

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

comment why these actions are repeated and if you plan to change how this works in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.
This is because MC only occurs after a full step, not intermediate step, and I am using a RK3 time stepper. That's not a good reason of course. What I should have here is some way to loop over all intermediate time steps of the time stepper and for each step do (AdvanceTime, RunEvents). Is there a simple way to do that for a generic timestepper with arbitrary number of intermediate steps?
We could also require MC to use forward Euler when not coupled to the fluid/metric. As that's what the code is actually doing at the moment, regardless of the choice of timestepper in the input files.

Copy link
Member

Choose a reason for hiding this comment

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

Okay I discussed with @nilsdeppe and we think you should be able to use something like the Actions::Label that are used to switch between Dg actions and subcell actions. So you would surround the MC actions with two labels (e.g. BeginMonteCarlo and EndMonteCarlo). Then you can add an action immediately before the BeginMonteCarlo label similar to evolution::dg::subcell::Actions::SelectNumericalMethod that will test whether the time substep is 0 and choose which label to jump to. This then will be independent of the time stepper choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I got it to work, but please look at the new Labels.hpp and TriggerMonteCarlo.hpp files as well as the action list to check that it is what you had in mind. These are the main changes in the new version of the PR (the other comments were fairly simple to take care of).

[&box](const auto* const data_or_solution) {
static constexpr size_t dim = System::volume_dim;
const double initial_time = db::get<::Tags::Time>(box);
const size_t num_grid_points =
Copy link
Member

Choose a reason for hiding this comment

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

You've already retrieved this above, so just capture it in the lambda

const Parallel::GlobalCache<Metavariables>& /*cache*/,
const ArrayIndex& /*array_index*/, ActionList /*meta*/,
const ParallelComponent* const /*meta*/) {
const size_t num_grid_points =
Copy link
Member

Choose a reason for hiding this comment

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

This assumes that Tags::ActiveGrid is ActiveGrid::Subcell. Instead check what the value is and either ERROR or update to handle correctly if it is ActiveGrid::Dg

make_not_null(&box), std::move(all_packets));

const unsigned long seed =
std::random_device{}(); // static_cast<unsigned long>(time(NULL));
Copy link
Member

Choose a reason for hiding this comment

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

remove commented code

@@ -131,14 +132,30 @@ struct TimeStepMutator {
const DirectionalIdMap<Dim, std::optional<DataVector>>&
cell_light_crossing_time_ghost = mortar_data.cell_light_crossing_time;

tnsr::iJJ<DataVector, 3, Frame::Inertial> d_inv_spatial_metric =
Copy link
Member

Choose a reason for hiding this comment

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

call the existing function in PointwiseFunctions/GeneralRelativity/DerivativeSpatialMetric.hpp

@@ -311,8 +321,8 @@ struct ReceiveDataForMcCommunication {
const DataVector& received_data_direction =
received_data[directional_element_id]
.ghost_zone_hydro_variables;
REQUIRE(received_data[directional_element_id]
.packets_entering_this_element == std::nullopt);
// REQUIRE(received_data[directional_element_id]
Copy link
Member

Choose a reason for hiding this comment

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

remove

@@ -368,8 +378,8 @@ struct ReceiveDataForMcCommunication {
received_data[directional_element_id]
.packets_entering_this_element;
// Temporary: currently no data for coupling to the fluid
REQUIRE(received_data[directional_element_id]
.ghost_zone_hydro_variables.size() == 0);
// REQUIRE(received_data[directional_element_id]
Copy link
Member

Choose a reason for hiding this comment

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

remove

using spacetime_variables_tag =
::Tags::Variables<gr::tags_for_hydro<volume_dim, DataVector>>;
using flux_spacetime_variables_tag = ::Tags::Variables<tmpl::list<>>;
// Hydro tags needed for background metric
Copy link
Member

Choose a reason for hiding this comment

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

why metric?

Copy link
Member

@kidder kidder left a comment

Choose a reason for hiding this comment

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

Looks good to me, you can squash

@ffoucart
Copy link
Contributor Author

Thanks. Will wait for the tests to complete here, then work on rebasing the next PR against these changes.

@ffoucart
Copy link
Contributor Author

Tests only failing due to Timeout in an unrelated test (I tried to rerun, but the same test failed twice)

@kidder kidder merged commit dfbc372 into sxs-collaboration:develop Jan 28, 2025
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants