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 initial condition of RadForce #627

Merged
merged 17 commits into from
May 12, 2024

Conversation

chongchonghe
Copy link
Contributor

@chongchonghe chongchonghe commented May 4, 2024

Description

The RadForce test can be improved to be more realistic and less ad hoc. Instead of initializing with the exact solution and letting the simulation maintain a static state, we start the simulation with a uniform density of rho0 and zero velocity. The left boundary condition is kept the same as before, but the right boundary is set to be foextrap. The end result is exactly the same as before: the radiation blows away the gas and the system comes to the static state given by the differential equation.

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.

@chongchonghe chongchonghe marked this pull request as ready for review May 4, 2024 14:22
@chongchonghe
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/RadForce/test_radiation_force.cpp Outdated Show resolved Hide resolved
src/RadForce/test_radiation_force.cpp Outdated Show resolved Hide resolved
src/RadForce/test_radiation_force.cpp Outdated Show resolved Hide resolved
src/RadForce/test_radiation_force.cpp Outdated Show resolved Hide resolved
src/RadForce/test_radiation_force.cpp Outdated Show resolved Hide resolved
@chongchonghe
Copy link
Contributor Author

I made this change in order to do a scaling test for the RHD solver, but the result is surprising: the R1 norm of the error does not change with decreasing dt (see table below). I don't know why this happens.

dt nx L1 norm elasped time
2e8 128 0.0004315252173 4.344377
1e8 128 0.0004296931455 6.812964
5e7 128 0.0004292276377 9.439731
2e8 64 0.0008842796415 2.600089
1e8 64 0.0008833405693 3.727579
2.5e7 64 0.0008808892701 12.810238

@chongchonghe
Copy link
Contributor Author

I just realized that this problem is in the diffusion limit, so it's not a good test for scaling of the time integration scheme. But in any case, this PR is worth merging.

@BenWibking
Copy link
Collaborator

I made this change in order to do a scaling test for the RHD solver, but the result is surprising: the R1 norm of the error does not change with decreasing dt (see table below). I don't know why this happens.

dt nx L1 norm elasped time
2e8 128 0.0004315252173 4.344377
1e8 128 0.0004296931455 6.812964
5e7 128 0.0004292276377 9.439731
2e8 64 0.0008842796415 2.600089
1e8 64 0.0008833405693 3.727579
2.5e7 64 0.0008808892701 12.810238

This will happen if the error is dominated by the spatial discretization rather than the time discretization. In that case, you only get significantly smaller errors if you increase the spatial resolution.

@BenWibking
Copy link
Collaborator

For a test problem, I don't think there's anything wrong with initializing to the exact solution. Is there any other benefit to changing this?

@chongchonghe
Copy link
Contributor Author

For a test problem, I don't think there's anything wrong with initializing to the exact solution. Is there any other benefit to changing this?

I just think a test that is initialized with the exact solution is not super reliable. Support you accidentally changed some of the problem parameters and the simulation finishes at t = 0, or the system is frozen for some reason and never evolves, the test will still pass. This PR shows you can start from a realistic initial condition and end with the exact analytic solution, so why not apply it?

@chongchonghe
Copy link
Contributor Author

@BenWibking Can you take a look at the setOutflowBoundaryLowOrder boundary condition in test_radiation_force.cpp? Why does it not work? The error message and where it comes from are copied below.

Assertion !S_filled.contains_nan() failed,

	AMREX_ASSERT(!S_filled.contains_nan()); // check ghost zones (usually this is caused by
						// forgetting to fill some components when
						// using custom Dirichlet BCs, e.g., radiation
						// variables in a hydro-only problem)

@BenWibking
Copy link
Collaborator

BenWibking commented May 7, 2024

@BenWibking Can you take a look at the setOutflowBoundaryLowOrder boundary condition in test_radiation_force.cpp? Why does it not work? The error message and where it comes from are copied below.

Assertion !S_filled.contains_nan() failed,

	AMREX_ASSERT(!S_filled.contains_nan()); // check ghost zones (usually this is caused by
						// forgetting to fill some components when
						// using custom Dirichlet BCs, e.g., radiation
						// variables in a hydro-only problem)

Ah, I am almost certain the cause is that I wrote this function to only fill the hydro variables. You will have to fill the ghost cells for the radiation variables yourself.

However, I forgot that this was all designed for the full Euler equations, i.e. where there is also an energy equation, which is missing for an isothermal EOS. So your original approach is probably fine. The serious issues only arise when the pressure boundary condition is not specified at an outflow boundary, but here it is not an issue since the pressure is not an independent variable. Somehow I didn't connect the dots here, sorry.

@chongchonghe
Copy link
Contributor Author

It shouldn't matter anyway, because the gas is definitely outflowing at the upper boundary. In my implementation of setOutflowBoundaryLowOrder, I used something similar to foextrap to pressure (= rho_R * cs^2).

So, the foextrap B.C. is okay? I'll change it back.

@chongchonghe
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

BenWibking
BenWibking previously approved these changes May 7, 2024
@BenWibking
Copy link
Collaborator

@chongchonghe Can you take a look at the GPU test that failed?

@chongchonghe
Copy link
Contributor Author

chongchonghe commented May 10, 2024

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/RadForce/test_radiation_force.cpp Outdated Show resolved Hide resolved
src/RadForce/test_radiation_force.cpp Outdated Show resolved Hide resolved
src/RadForce/test_radiation_force.cpp Outdated Show resolved Hide resolved
@chongchonghe
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/RadForce/test_radiation_force.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/RadForce/test_radiation_force.cpp Outdated Show resolved Hide resolved

for (std::string line; std::getline(fstream, line);) {
std::istringstream iss(line);
std::vector<double> values;

Check notice

Code scanning / CodeQL

Declaration hides variable Note test

Variable values hides another variable of the same name (on
line 230
).
@chongchonghe
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@chongchonghe chongchonghe added this pull request to the merge queue May 12, 2024
Merged via the queue into development with commit d7f4b43 May 12, 2024
21 checks passed
@chongchonghe chongchonghe deleted the chongchong/convergence-test branch May 14, 2024 15:33
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