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

Metal advection problem #712

Open
wants to merge 195 commits into
base: development
Choose a base branch
from

Conversation

aditivijayan
Copy link
Contributor

@aditivijayan aditivijayan commented Aug 10, 2024

Description

There was an issue in the test_sne.cpp, specifically in the addStrangSplitSources function.

Firstly, the momentum should have been referred as "HydroSystem::x1Momentum_index" rather than "x1mom".

Secondly, the state momentum variables need to be updated in two steps. Writing "state = state + rho*GradPhi" does not work. I am unsure why.

The problem compiles and runs on gadi.

Related issues

none

Checklist

Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an x inside the square brackets [ ] in the Markdown source below:

  • I have added a description (see above).
  • I have added a link to any related issues see (see above).
  • I have read the Contributing Guide.
  • I have added tests for any new physics that this PR adds to the code.
  • I have tested this PR on my local computer and all tests pass.
  • I have manually triggered the GPU tests with the magic comment /azp run.
  • I have requested a reviewer for this PR.

aditivijayan and others added 30 commits January 20, 2023 07:59
src/problems/MetalAdvectionProblem/test_sne.cpp Outdated Show resolved Hide resolved
src/problems/MetalAdvectionProblem/test_sne.cpp Outdated Show resolved Hide resolved
src/problems/MetalAdvectionProblem/test_sne.cpp Outdated Show resolved Hide resolved
src/problems/MetalAdvectionProblem/CMakeLists.txt Outdated Show resolved Hide resolved
src/problems/MetalAdvectionProblem/data_sets.dat Outdated Show resolved Hide resolved
@BenWibking
Copy link
Collaborator

BenWibking commented Aug 10, 2024

In state = state + rho*GradPhi, I think the term should have a negative sign, since $g = -\nabla \phi$.
So I think it should be: state += dt * (-rho*GradPhi).

But I think it makes sense to do it the way you've done it in the code, since you need the new momentum value in any case in order to compute the new total energy.

@BenWibking
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@BenWibking
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@BenWibking BenWibking changed the title Setonix gas grav Metal advection problem Aug 10, 2024
@BenWibking
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).


if ((x3Mom_edge * normal) < 0) { // gas is inflowing
x3Mom_edge = -1. * consVar(i, j, kedge, HydroSystem<NewProblem>::x3Momentum_index);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This implementation of the diode boundary condition is not correct.

When the gas is inflowing, you have to reflect all of the cells about the boundary face. For example:

  • the khi+1 ghost cell needs to be based on the khi cell, whereas
  • the khi+2 ghost cell needs to be based on the khi-1 cell,
  • the khi+3 ghost cell needs to be based on the khi-2 cell, and
  • the khi+4 ghost cell needs to be based on the khi-3 cell.

@BenWibking
Copy link
Collaborator

For future reference, I've successfully compiled and run commit 9bb4588a254745cb52a6c4484ad20deb482895b9 on Frontier with the metal_problem_regression.in input on 1 node (this is only a 32 x 32 x 256 problem).

Modules:

wibking@login03:/lustre/orion/ast146/scratch/wibking/quokka> module list

Currently Loaded Modules:
  1) Core/24.07           5) craype-network-ofi        9) rocm/5.7.1           13) cray-hdf5/1.12.2.9
  2) craype/2.7.31        6) PrgEnv-cray/8.5.0        10) cce/17.0.0           14) cmake/3.27.9
  3) cray-dsmml/0.2.2     7) craype-x86-trento        11) cray-libsci/23.12.5  15) emacs/29.3
  4) libfabric/1.15.2.0   8) craype-accel-amd-gfx90a  12) cray-mpich/8.1.28    16) cray-python/3.11.5

@BenWibking
Copy link
Collaborator

BenWibking commented Aug 12, 2024

I've also successfully run a 256 x 256 x 1024 problem on 32 nodes for 1e6 timesteps without any issues on Frontier. Same commit and modules as above.

@BenWibking
Copy link
Collaborator

BenWibking commented Aug 14, 2024

On Frontier, the printfs on the GPU all appear at the very end of the log file, after the simulation is finished running. To avoid this problem in the future, these printfs should probably be moved to the CPU.


printf("The total number of SN gone off=%d\n", cum_sn);
Real Rpds = 14. * std::pow(state(i, j, k, HydroSystem<NewProblem>::density_index) / Const_mH, -3. / 7.);
printf("Rpds = %.2e pc\n", Rpds);
Copy link
Collaborator

Choose a reason for hiding this comment

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

On Frontier, printf from the GPU is buffered until the very end of the simulation. For future runs, it would help to move this outside of the amrex::ParallelFor (and therefore back onto the CPU).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants