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

Define advecting pulse test problem. #463

Merged
merged 81 commits into from
Jan 6, 2024

Conversation

chongchonghe
Copy link
Contributor

Description

Defined the advecting pulse test problem (Krumholz+2007). This test gives exactly the same result (visually) as in CASTRO II paper and consistent result between non-advecting and advecting case, demonstrating the code's ability to resolve diffusion limit as well as capture radiation advection in this limit.

Notes:

  • Based on Add-O(beta-tau)-terms branch

Checklist

  • 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 and others added 18 commits November 15, 2023 12:45
First attempt. With a22=0 and a32=0, it successfully reduces
to SSP-RK2 scheme. However, with a22=1 and 0<a32<=0.5 (IMEX PD-ARS), the
RadShock and Coupling test failed.
the Asymptotic Marshak test.

- Fixed a problem in implementing IMEX PD-ARS:
  Egas and Fgas should update by 0.4 dt Src instead of dt Src
  in the first stage.
- Add RadhydroPulse test problem
- Add the O(beta tau) terms to the matter-radiation exchange
- Add the RadhydroUniformAdvecting test for this new physics
- Modified boundary condition in RadhydroShock problem
- Passed all tests
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/RadhydroPulse/test_radhydro_pulse.cpp Outdated Show resolved Hide resolved
src/RadhydroPulse/test_radhydro_pulse.cpp Outdated Show resolved Hide resolved
src/RadhydroPulse/test_radhydro_pulse.cpp Outdated Show resolved Hide resolved
src/RadhydroPulse/test_radhydro_pulse.cpp Outdated Show resolved Hide resolved
@chongchonghe
Copy link
Contributor Author

Almost ready for merging after Add-O(beta-tau)-terms is accepted. Before merging, I need to think about a more robust way to validate the result. Currently, I compare the solutions in advecting and non-advecting cases visually and confirm they are consistent. However, to make it more convincing, we need to compare the shifted advecting result with the non-advecting result in one figure. @BenWibking What's your idea on how to achieve this? Is it possible to run a simulation with v=0 via sim.evolve(), copy the result, then reset the initial condition with v>0, and run it again, in the same cpp file?

@chongchonghe
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@chongchonghe
Copy link
Contributor Author

Almost ready for merging after Add-O(beta-tau)-terms is accepted. Before merging, I need to think about a more robust way to validate the result. Currently, I compare the solutions in advecting and non-advecting cases visually and confirm they are consistent. However, to make it more convincing, we need to compare the shifted advecting result with the non-advecting result in one figure. @BenWibking What's your idea on how to achieve this? Is it possible to run a simulation with v=0 via sim.evolve(), copy the result, then reset the initial condition with v>0, and run it again, in the same cpp file?

As I think about it, this is probably not necessary. Radiation advection is strictly tested in the RadhydroUniformAdvecting test, so there is no need to validate it here. Since there is no analytic solution to this problem (as we are aware of), we can just keep it as it is.

So, ready to merge after Add-O(beta-tau)-terms branch is merged.

@chongchonghe chongchonghe self-assigned this Dec 4, 2023
@BenWibking
Copy link
Collaborator

Almost ready for merging after Add-O(beta-tau)-terms is accepted. Before merging, I need to think about a more robust way to validate the result. Currently, I compare the solutions in advecting and non-advecting cases visually and confirm they are consistent. However, to make it more convincing, we need to compare the shifted advecting result with the non-advecting result in one figure. @BenWibking What's your idea on how to achieve this? Is it possible to run a simulation with v=0 via sim.evolve(), copy the result, then reset the initial condition with v>0, and run it again, in the same cpp file?

Yes, this is possible. You can create a new sim object and run a new simulation after the first one is done. Then you can copy the final state from both and compare them. That would be the best way to do this, in my opinion.

@BenWibking
Copy link
Collaborator

I think this PR is merging into the wrong branch... it says: "chongchonghe wants to merge 4 commits into Add-O(beta-tau)-terms from advecting_pulse_test_new"

Do you mean to merge this into development instead?

@chongchonghe
Copy link
Contributor Author

I think this PR is merging into the wrong branch... it says: "chongchonghe wants to merge 4 commits into Add-O(beta-tau)-terms from advecting_pulse_test_new"

Do you mean to merge this into development instead?

OK, same as the other one. I'll fix it.

@chongchonghe
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@chongchonghe
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@chongchonghe
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@chongchonghe
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@BenWibking
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@chongchonghe
Copy link
Contributor Author

OK. The RadhydroPulse test takes way too long to run in 3D on GPU, so I have to turn it off. I keep it on only when compiled with 1D. I'll test it with 3D GPU on gadi manually and ensure it gives the right result.

/azp run

@chongchonghe
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@BenWibking
Copy link
Collaborator

BenWibking commented Jan 6, 2024

It looks like there's an instability present in the non-advecting velocity (see below), but that can be fixed in a separate PR.
radhydro_pulse_velocity.pdf

Copy link
Collaborator

@BenWibking BenWibking left a comment

Choose a reason for hiding this comment

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

LGTM

@BenWibking BenWibking added this pull request to the merge queue Jan 6, 2024
Merged via the queue into development with commit 5451412 Jan 6, 2024
13 checks passed
@chongchonghe chongchonghe deleted the advecting_pulse_test_new branch April 10, 2024 07:44
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