IAS-Astrophysics / athenak

Performance-portable version of the Athena++ astrophysical AMR-MHD code using Kokkos.
BSD 3-Clause "New" or "Revised" License
18 stars 11 forks source link

GRMHD in dynamical spacetimes - [merged] #571

Closed jmstone closed 1 month ago

jmstone commented 3 months ago

In GitLab by @jfields7 on Jun 13, 2024, 12:54

Merges z4c-matter-rebase -> master

Major changes

This long-delayed merge request adds a new module for GRMHD in dynamical spacetimes. The number of changes are quite extensive, but they are mostly independent of functionality elsewhere in the code. Here is a brief summary of the most important changes:

Unfinished tasks

There are a number of tasks still left to do. However, the code has introduced enough new features that I figured it would be better to merge back sooner rather than later. Here's what is still left:

Validation

Here are a few tests showing that the code works as expected:

balsara-1 balsara-4

rho_ft

rho_ft

h22

The BNS test achieves approximate second-order convergence of the waveform phase during the inspiral as expected. The merger time differs from GR-Athena++ by $\Delta t \approx 0.05~\mathrm{ms}$. The results do diverge somewhat in the post-merger phase, particularly in predicted collapse time, but this a combination of insufficient resolution and the extreme sensitivity of collapse to initial conditions.

jmstone commented 3 months ago

In GitLab by @jfields7 on Jun 13, 2024, 13:03

added 2 commits

Compare with previous version

jmstone commented 3 months ago

In GitLab by @jfields7 on Jun 13, 2024, 13:09

added 1 commit

Compare with previous version

jmstone commented 3 months ago

In GitLab by @jfields7 on Jun 13, 2024, 13:16

added 1 commit

Compare with previous version

jmstone commented 3 months ago

In GitLab by @jfields7 on Jun 13, 2024, 13:22

added 1 commit

Compare with previous version

jmstone commented 3 months ago

In GitLab by @jfields7 on Jun 13, 2024, 13:24

added 1 commit

Compare with previous version

jmstone commented 3 months ago

In GitLab by @jfields7 on Jun 13, 2024, 13:27

added 1 commit

Compare with previous version

jmstone commented 3 months ago

In GitLab by @jfields7 on Jun 14, 2024, 11:24

marked this merge request as ready

jmstone commented 3 months ago

In GitLab by @jfields7 on Jun 14, 2024, 11:24

requested review from @jmstone216, @Hengrui_Zhu, and @dradice

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 14, 2024, 16:52

added 1 commit

Compare with previous version

jmstone commented 2 months ago

In GitLab by @jmstone216 on Jun 15, 2024, 09:12

This is a milestone! Congrats on getting it all done. There is a lot of code to review and it will take me a few days to digest everything. But two immediate thoughts are: (1) could we have extended the flux and FOFC functions in the existing hydro/mhd classes so they could handle the more general EOS. More general EOS is useful in stationary spacetimes as well, and this might reduce code duplication, and (2) I'd like to understand the "task orchestrator" in NumericalRelativity. If it is just a general task list constructor, can we just use it everywhere? Shouldn't it be part of the task list infrastructure?

I am a little worried that rather than building one integrated code framework we are essentially building two completely separate GRMHD codes that only share the mesh and I/O infrastructure. So I would like to make sure we integrate everything together as much as possible, even if it requires making changes to the existing hydro/mhd/eos.

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 15, 2024, 11:03

Support for the general EOS would require some pretty heavy changes, I think, but it's doable in principle. The flux/FOFC functions would need to be templated over the EOS and error policies like they are for DynGR, and the way the ADM variables are captured also needs to be changed (rather than directly capturing them, we would need to do some conditional aliasing, similar to how many of the pgens handle HD/MHD).

Right now the task orchestrator is hard-coded for DynGR and Z4c (which is why I just called it NumericalRelativity), but it would be very easy to extend this to non-NR physics and just make it part of the existing task list infrastructure. In the first iteration of this class, queuing tasks was a very clumsy, tedious operation which required ~3 lines of code per task, so it was really only practical for physics modules that had complex optional dependencies on other modules. This is no longer the case, so it might be worthwhile to integrate it into the existing task list infrastructure.

However, this class does need to be refactored if we plan to do this. Right now tasks are stored in a giant enum (numrel::TaskName), and each task is compared to a divider value in the enum (MHD_NTASKS or Z4c_NTASKS) to figure out what physics it implements. This is very simple, but it's not a very extensible design. For example, right now the Z4c RK update is queued as follows:

pnr->QueueTask(&Z4c::ExpRKUpdate, this, Z4c_ExplRK, "Z4c_ExplRK", Task_Run, {Z4c_SomBC}, {MHD_EField});

NumericalRelativity sees Z4c_SomBC and recognizes it as a Z4c task because MHD_NTASKS < Z4c_SomBC < Z4c_NTASKS, and it sees MHD_EField as an MHD task because MHD_EField < MHD_NTASKS. Z4c_SomBC is added as a required dependency, and it sees MHD_EField as an optional dependency. If the MHD module is enabled, it adds it to the final dependency list for Z4c_ExplRK.

The string we pass is an ID for debugging purposes, but I think we could use string IDs in place of enumerators altogether. I'm imagining something like the following:

pnr->QueueTask(&Z4c::ExpRKUpdate, this, "Z4c:ExplRK", Task_Run, {"Z4c:SomBC"}, {"MHD:EField"});

NumericalRelativity (which we could rename TaskListFactory or TaskListOrchestrator) would split each string token at the ':' character and do a string comparison on the first half to determine the right physics options. Therefore, it would see "Z4c:SomBC", extract out "Z4c", and recognize that it's a Z4c task. It would then do the same thing to "MHD:EField" to get "MHD", determine that it needs the MHD module, and enable it if it's enabled. This way the task orchestrator just needs to know about each physics module rather than each physics task. One additional refactoring step could replace the Task_Start/Run/End enumerator with the string key for the task list map, and then it would be completely generic for all tasks.

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 15, 2024, 12:00

A few more notes on potentially merging the MHD infrastructure:

  1. The MHD class itself would probably need to be redesigned similar to how DynGR is at the moment. This is definitely doable, but it does add a fair bit of complexity to the design.
  2. The general EOS infrastructure in PrimitiveSolver is really targeted toward relativistic fluids. Newtonian fluids could use it in its current form, but many of the thermodynamic quantities (enthalpy, sound speed, energy density, etc.) are calculated in their relativistic form with the implicit assumptions that $c=1$, rest-mass contributions are included, etc. The EOS infrastructure also includes error-handling policies for things like excessive magnetization, flooring the conserved variables, etc. For a similar reason, many of these features would be strictly off-limits to Newtonian fluids. If we go this route, a first step might still include hard-coded EOS calls for the Newtonian case.

I think there's a lot of value in trying to merge the infrastructure where possible, but I also think it's a longer-term project that we need to think about very carefully.

jmstone commented 2 months ago

In GitLab by @jmstone216 on Jun 18, 2024, 13:59

OK, this is such a major update I only want to make high-level suggestions for now. As I digest the code I might make more detailed suggestions in the future, but I think we can merge before that and continue to polish/integrate the code in future commits.

My high level suggestions are the following: (1) It seems only GRMHD is implemented for now. Should src/dyngr really be named src/dyn_grmhd? (2) Could we move /numerical-relativity to src/tasklist? I believe generalizing it so that it can be used in other places is a good idea, but that is going to require further discussion and best left for the future. (3) Similarly, could /adm be moved to src/coordinates? And /tmunu moved to /dyngr? I'd like to keep the /src directory as concise as possible, and these directories only contain a single class that is related to what is in these other directories. (4) You mention updating outputs so that eint->pressure when appropriate. In fact whether we use pressure, internal energy, or temperature as the primitive variable needs to be generalized throughout as this is an issue for general EOS without relativity. Again, something for the future.

My low-level comments are: (1) add comments in config.hpp.in as to what LORENE and ELLIPTICA are used for (2) add comment as to what scratch_fix.patch is for

Once these are addressed I am happy to approve. Once David and Hengrui have approved I am happy to merge!

It may be time to start planning another in-person meeting in the near future to discuss refactoring the task list queuing and integrating general EOS design into the HARM-like and NR solvers.

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 18, 2024, 14:54

High level comments:

  1. I think this is a good idea. It's not immediately clear what dyngr is.
  2. Yes, that seems reasonable to me.
  3. We have some extensions planned for the ADM class soon which may add additional files. Nevertheless, we can always move it back out into its own folder. Regarding tmunu, it may be more appropriate to move it into adm or z4c. DynGR is currently the only physics module that adds non-zero terms to these sources, but in principle the upcoming neutrino radiation module or other physics modules could modify Tmunu as well.

Low level comments:

  1. Will do.
  2. Also will do. The patch fixes a bug with uninitialized CUDA scratch locks in Kokkos 4.1, which sometimes causes level 1 scratch memory to result in crashes. When we're ready to bump the code to Kokkos 4.2 or 4.3, we can get rid of this patch.
jmstone commented 2 months ago

In GitLab by @dradice on Jun 19, 2024, 09:31

approved this merge request

jmstone commented 2 months ago

In GitLab by @dradice on Jun 19, 2024, 09:41

I have reviewed the code and, given the results on all of the tests, I think it is correct and ready to be merged. The EOS interface might evolve in the future, particularly as we look forward to include more physics, but the GRMHD solver is stable. Once we are happy with the EHT test, I would like to have a discussion about merging this with the HARM formulation, to avoid having two different and incompatible GRMHD solvers.

If there is interest, it would be nice to have a small developer meeting in September here at Penn State. If that works for Jim and Hengrui, we can cover the local expenses.

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 24, 2024, 11:31

added 4 commits

Compare with previous version

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 24, 2024, 11:31

reset approvals from @dradice by pushing to the branch

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 24, 2024, 11:33

added 1 commit

Compare with previous version

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 25, 2024, 13:30

added 3 commits

Compare with previous version

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 26, 2024, 15:30

added 2 commits

Compare with previous version

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 26, 2024, 15:32

An update:

  1. DynGR and related classes have all been renamed to use DynGRMHD instead. Similarly, dyngr has been changed to dyn_grmhd in all filenames, including the dyngr directory. I have not gone through and updated every instance of pdyngr to pdyngrmhd because it seems a bit verbose, but this is something I could change if desired.
  2. numerical-relativity has been merged into the tasklist directory.
  3. adm has been moved to coordinates for the time being. tmunu now lives inside z4c.
  4. The LORENE and ELLIPTICA definitions have been removed because they were redundant; their respective pgens won't compile without these libraries, anyway.
  5. I've added a comment inside the README about the scratch_fix.patch file.

I think this addresses most of the high-level and low-level comments. I'll pull in the new changes from the master branch to fix conflicts and linting errors, and then, if there are no more comments, the code should be ready to merge.

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 26, 2024, 15:35

added 1 commit

Compare with previous version

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 26, 2024, 19:36

added 51 commits

Compare with previous version

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 28, 2024, 11:34

marked this merge request as draft

jmstone commented 2 months ago

In GitLab by @jfields7 on Jun 28, 2024, 11:36

I've temporarily marked this merge request as a draft again. Though the code still passes the CI pipeline, further testing shows that I've broken something during these updates.

jmstone commented 2 months ago

In GitLab by @jfields7 on Jul 10, 2024, 16:26

added 4 commits

Compare with previous version

jmstone commented 2 months ago

In GitLab by @jfields7 on Jul 12, 2024, 08:48

marked this merge request as ready

jmstone commented 1 month ago

In GitLab by @jfields7 on Jul 23, 2024, 13:18

added 3 commits

Compare with previous version

jmstone commented 1 month ago

In GitLab by @jmstone216 on Jul 31, 2024, 14:21

mentioned in commit 4873d02b58cf810952b4496a9838fd8515b8a29a