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

Add Preliminary MHD Integrator #220

Merged
merged 19 commits into from
Jan 10, 2023

Conversation

bcaddy
Copy link
Collaborator

@bcaddy bcaddy commented Dec 20, 2022

All non-MHD tests pass except those noted in issues #218 and #219 which also don't pass in dev.

This commit primarily adds support for MHD to the Van Leer 3D integrator along with myriad small fixes and modifications. All new features have tests as well.

The MHD implementation still has bugs but needs to be merged in before we reformat the code so that the merging doesn't provoke too many merge conflicts.

MHD

  • Add MHD support to Van Leer 3D integrator
  • All MHD code is in the mhd namespace with sub-namespaces when relevant (e.g. mhd::utils)
  • Add kernel to compute the CT electric fields and tests for it
  • Add kernel to update the magnetic field and tests for it
  • Add MHD support to the PCM reconstruction
  • Add new Linear_Wave initial condition type
  • Add ctElectricFields global device array
  • Set all MHD input parameters to default to zero
  • Remove incorrect MHD specializations in the boundary conditions
  • Add check to make sure MHD has at least 3 ghost cells
  • Add functions to compute the maximum magnetic divergence, report it, and exit if it's too large
  • Clean up make.type.mhd and enable cuda error checking
  • Add example parameter files for
    • Alfven wave
    • Fast magnetosonic wave
    • Slow magnetosonic wave
    • Ryu & Jones 1995 shock tubes 1a and 2a
  • Add the following input parameters for generalized linear waves
    • EigenVec_rho
    • EigenVec_MomentumX
    • EigenVec_MomentumY
    • EigenVec_MomentumZ
    • EigenVec_E
    • EigenVec_Bx
    • EigenVec_By
    • EigenVec_Bz
    • pitch
    • yaw
  • Added MHD system tests for:
    • Hydro only constant state test
    • MHD constant state
    • Sod test that works with PCM (this will be merged with the hydro test when MHD supports PPMC)
    • All 4 MHD linear waves
    • MHD Einfeldt strong rarefaction

HLLD Riemann Solver

  • Updated interface state indexing for new format
  • Added documentation
  • Moved dot product function to math_utils

Testing

  • Add new method, runL1ErrorTest, to SystemTestRunner that computes the L1 error compared to the initial conditions. Ideal for wave tests
  • Updated hydro wave tests to use new runL1ErrorTest method

Reductions

  • Add new FP atomic max that works on CUDA or ROCm
  • Update time step calculation to use the grid reduction with atomic and DeviceVector
  • Updated tests to match
  • Remove dev_dti, host_dti_array, and dev_dti_array global variables

Utilities

Cuda Utilities

  • Add function initGpuMemory which initializes GPU memory so the CUDA compute sanitizer doesn't complain about it. Used to initialize all GPU arrays that I know of.
  • Add struct AutomaticLaunchParams which is a thin wrapper over the occupancy API. Primarily intended for reductions where performance is sensitive to the number of blocks but could be used for any kernel launch

DeviceVector

  • New option to initialize memory

Math Utilities

New namespace and file for math utilities. Currently contains a semi-general rotation and dot product functions

MHD Utilities

  • The mhd::utils::computeEnergy function now works properly with either MHD or hydro and returns the appropriate energy

Other

  • Add a function to_string_exact to convert floating point numbers to a string such that it can be exactly deserialized back from a string to the same floating point number. Only used in tests currently
  • Commit 379b86e in this PR should Fix Issue Dust make type doesn't build on Crusher #219 as well.

@bcaddy
Copy link
Collaborator Author

bcaddy commented Dec 20, 2022

This is currently a draft and not ready for merging but wanted to make the changes available for code review as the things that are left to do are minor and should be done shortly.

To do

  • Rebase off of dev again now that PR Grid Enum - dev #213 is merged
  • Replace current indexing in MHD code with grid enums

@bcaddy
Copy link
Collaborator Author

bcaddy commented Dec 20, 2022

Documentation for the runL1Error method has been added to the wiki

This commit primarily adds support for MHD to the Van Leer 3D
integrator along with myriad small fixes and modifications. All new
features have tests as well.

The MHD implementation still has bugs but needs to be merged in before
we reformat the code so that the merging doesn't provoke too many
merge conflicts.

MHD

- Add MHD support to Van Leer 3D integrator
- All MHD code is in the `mhd` namespace with sub-namespaces when
  relevant (e.g. `mhd::utils`)
- Add kernel to compute the CT electric fields and tests for it
- Add kernel to update the magnetic field and tests for it
- Add MHD support to the PCM reconstruction
- Add new `Linear_Wave` initial condition type
- Add `ctElectricFields` global device array
- Set all MHD input parameters to default to zero
- Remove incorrect MHD specializations in the boundary conditions
- Add check to make sure MHD has at least 3 ghost cells
- Add functions to compute the maximum magnetic divergence, report it,
  and exit if it's too large
- Clean up make.type.mhd and enable cuda error checking
- Add example parameter files for
  - Alfven wave
  - Fast magnetosonic wave
  - Slow magnetosonic wave
  - Ryu & Jones 1995 shock tubes 1a and 2a
- Add the following input parameters for generalized linear waves
  - EigenVec_rho
  - EigenVec_MomentumX
  - EigenVec_MomentumY
  - EigenVec_MomentumZ
  - EigenVec_E
  - EigenVec_Bx
  - EigenVec_By
  - EigenVec_Bz
  - pitch
  - yaw
- Added MHD system tests for:
  - Hydro only constant state test
  - MHD constant state
  - Sod test that works with PCM (this will be merged with the hydro
    test when MHD supports PPMC)
  - All 4 MHD linear waves
  - MHD Einfeldt strong rarefaction

Testing

- Add new method, `runL1ErrorTest`, to `SystemTestRunner` that
  computes the L1 error compared to the initial conditions. Ideal for
  wave tests
- Updated hydro wave tests to use new `runL1ErrorTest` method

HLLD Riemann Solver

- Updated interface state indexing for new format
- Added documentation
- Moved dot product function to `math_utils`

Reductions

- Add new FP atomic max that works on CUDA or ROCm
- Update time step calculation to use the grid reduction with atomic
  and DeviceVector
- Updated tests to match
- Remove dev_dti, host_dti_array, and dev_dti_array global variables

Utilities

Cuda Utilities

- Add function `initGpuMemory` which initializes GPU memory so the
  CUDA compute sanitizer doesn't complain about it. Used to initialize
  all GPU arrays that I know of.
- Add struct `AutomaticLaunchParams` which is a thin wrapper over the
  occupancy API. Primarily intended for reductions where performance
  is sensitive to the number of blocks but could be used for any
  kernel launch

DeviceVector

- New option to initialize memory

Math Utilities

New namespace and file for math utilities. Currently contains a
semi-general rotation and dot product functions

MHD Utilities

- The `mhd::utils::computeEnergy` function now works properly with
  either MHD or hydro and returns the appropriate energy

Other

- Add a function `to_string_exact` to convert floating point numbers
  to a string such that it can be exactly deserialized back from a
  string to the same floating point number. Used in tests currently
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@bcaddy
Copy link
Collaborator Author

bcaddy commented Dec 20, 2022

Converting MHD to using grid_enum is done so I'm converting this from a draft to a full PR.

@alwinm, switching to using the enum was pretty easy. Could you take a look at my additions to the enum and my general usage? I think it's what we talked about but I'd like confirmation and feedback.

@bcaddy bcaddy marked this pull request as ready for review December 20, 2022 23:07
@bcaddy
Copy link
Collaborator Author

bcaddy commented Dec 20, 2022

FYI, I think github actions are having some issues at the moment. Some jobs are getting shutdown signals from somewhere. I double checked and everything builds fine on C-3PO and the containers running on my machine. For now I'm just restarting failed jobs until they pass.

@alwinm alwinm changed the title Add Preliminary MHD Intergrater Add Preliminary MHD Integrator Dec 21, 2022
Copy link
Collaborator

@evaneschneider evaneschneider left a comment

Choose a reason for hiding this comment

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

I've only made it through the integrator, but figured I'd leave what comments I have so far.

src/grid/grid3D.cpp Outdated Show resolved Hide resolved
C.momentum_x[id] = rho*vx;
C.momentum_y[id] = rho*vy;
C.momentum_z[id] = rho*vz;
C.Energy[id] = mhd::utils::computeEnergy(P, rho, vx, vy, vz, Bx, By, Bz, gama);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this function is supposed to compile even if MHD is turned off. Does that mean that everything in the mhd:utils namespace gets compiled if MHD is turned off? I ask in part because there is also a hydro utils function that calculates the energy, and I'm not sure when it makes sense to use one versus the other. I guess I would have thought that lines like this would be approached the way they are in the time step calculation, where one function gets called for the hydro version, and the MHD version is wrapped in a macro.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only this function is compiled if MHD is turned off. mhd::utils::computeEnergy returns the correct hydro or mhd energy depending on what flags are enabled. IMO we really need to unify these utility functions. We should be able to just call "computeEnergy" or something similar and it will automatically choose the correct way to compute the energy, then there can be separate functions if you need a specific one. Otherwise at some point someone is going to be computing the hydro energy when they want the MHD energy or vice versa

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realize I thought your question was on the mhd::utils::computeEnergy function not the Riemann ICs function. My earlier anserwe still stands though, mhd::utils::computeEnergy returns the correct energy whether or not MHD is on.

src/hydro/hydro_cuda.cu Outdated Show resolved Hide resolved
src/integrators/VL_3D_cuda.cu Show resolved Hide resolved
All the magnetic divergence stuff now lives in the magnetic_divergence*
files. I also simplified the functions a bit
Reduced the number of jobs launched to match the number of threads
available. Will hopefully reduce memory pressure in github actions builds
@bcaddy
Copy link
Collaborator Author

bcaddy commented Jan 2, 2023

I think I figured out the issue with builds randomly failing. The error ended up being too many resources consumed. I reduced the number of jobs that Make launched and it seems to have fixed it

Change a few comments and make dust compile on crusher
src/mhd/ct_electric_fields.cu Outdated Show resolved Hide resolved
src/mhd/magnetic_divergence.cu Outdated Show resolved Hide resolved
src/mhd/magnetic_update.cu Show resolved Hide resolved
src/reconstruction/pcm_cuda.cu Outdated Show resolved Hide resolved
src/utils/gpu.hpp Outdated Show resolved Hide resolved
The divergence calculations have all been moved out of Grid3D and into
the `mhd` namespace. There wasn't any real reason for it to be in
Grid3D and now all the MHD stuff is in the `mhd` namespace
`mhd::utils::cellCenteredMagneticFields` now returns the centered
magnetic fields using structured binding so the resultant values can be
declared const.
We're not entirely sure how to do this and since it probably isn't
required we're removing support for now.

Also, slightly refactored the MHD part of the timestep calculation to
combine two #ifdef statements into one
@evaneschneider evaneschneider merged commit 6fb50f8 into cholla-hydro:dev Jan 10, 2023
@bcaddy bcaddy deleted the dev-mhd-integrator branch February 3, 2023 16:37
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.

3 participants