Skip to content

Shearing box self gravity #325

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

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from

Conversation

vdbma
Copy link
Collaborator

@vdbma vdbma commented Feb 25, 2025

This adds the shearing box boundary conditions for the self-gravity potential.

Some notes that need to be discussed:

  • Compared to the fluid fluxes reconstruction, I do not use the "slope limited" reconstruction as it does not seem relevant for the potential
  • In laplacian.cpp line 43 the isPeriodic variable is set but seems to never be used
  • For some reason the MPI version of the added test fails with error above tolerance level (1.6e-08), I don't know why this is.
  • I added a note for the uniform initial state in periodic setups (=vacuum).

@vdbma vdbma force-pushed the ShearingBoxSelfGravity branch from 6f57c50 to 46512dd Compare February 25, 2025 13:25
@glesur
Copy link
Contributor

glesur commented Mar 13, 2025

a few comments on the fly:

  • can you revert your changes to the submodules kokkos and reference so that the tests will pass, and we don't downgrade kokkos accidentally
  • I agree that slope limited reconstruction is probably not required for the gravitational potential.
  • I will have a look at your test and why it might fail. Have you tried to run this on different architectures without MPI? Do the results match?

@glesur glesur self-requested a review March 13, 2025 12:20
@glesur glesur added the enhancement New feature or request label Mar 13, 2025
@vdbma
Copy link
Collaborator Author

vdbma commented Mar 17, 2025

For the test: I tried it on A100 (no MPI) and :

  • the custom testidefix.py test yields the exact same error and passes
  • the non-regression test fails with similar error (but slightly different ~2e-8)

@glesur
Copy link
Contributor

glesur commented Mar 19, 2025

Some of the variables in the test were not initialised, so this can certainly lead to different results.

In addition, the test provided is not great (as indicated by the large error of testidefix.py), as it seems like it assumes perfect advection of the potential by the shear, but at the same time, density changes because of sound wave propagation, and there is this weird VX1 that is I believe a residue from the hydro shearing wave test (is it?). This should really be cleaned up.

Can't we have a shearing-box+self-gravity following a self-gravitating shearing wave? Would make more sense IMHO.

@vdbma
Copy link
Collaborator Author

vdbma commented May 5, 2025

I added a test for the shearing wave. I mostly copied the setup section 4 of from Paardekooper 2012. But there is a key difference explaining the difference with this test and their Fig 3: This setup is purely 2D, not with a razor thin disc. This is why the ODE in the python script is different from their eq 89 (there is a factor 2/k different) and the produced result is not the same.

On my side it passes with MPI and on a single A100.

I can remove the test I first implemented, as the shearing wave is a better one.

This test also passes with no self gravity, but I did not include that version, to avoid redundancy with the already exist shearing box test. Let me know if you think it should be included as well.

@vdbma vdbma force-pushed the ShearingBoxSelfGravity branch from e8ece7c to 29cbeae Compare June 16, 2025 11:11
@vdbma
Copy link
Collaborator Author

vdbma commented Jun 16, 2025

Rebased to have #346

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants