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

[WIP] Gravity with subcycling #499

Draft
wants to merge 77 commits into
base: development
Choose a base branch
from

Conversation

psharda
Copy link
Contributor

@psharda psharda commented Jan 14, 2024

Description

Continues #20 to add gravity with subcycling

Related issues

#20

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.

src/Gravity_impl.hpp 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

There were too many comments to post at once. Showing the first 10 out of 143. Check the log or trigger a new build to see more.

src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.hpp Show resolved Hide resolved
src/Gravity.hpp Outdated Show resolved Hide resolved
src/Gravity.hpp Outdated Show resolved Hide resolved
src/Gravity.hpp Outdated Show resolved Hide resolved
src/Gravity.hpp Outdated Show resolved Hide resolved
src/Gravity.hpp Show resolved Hide resolved
src/Gravity.hpp Outdated Show resolved Hide resolved
@@ -139,6 +141,7 @@ template <typename problem_t> class RadhydroSimulation : public AMRSimulation<pr
amrex::Long radiationCellUpdates_ = 0; // total number of radiation cell-updates

amrex::Real Gconst_ = C::Gconst; // gravitational constant G
std::unique_ptr<Gravity<problem_t>> gravitySolver;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I created the gravity solver object here, but I apparently never initialized it. In the test problem in test_poisson.cpp, I create a separate Gravity<problem_t> object just for testing purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do I remove this line?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is needed in either AMRSimulation or RadhydroSimulation. Which one is up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay lets keep it here as it is then.

// solve Poisson equation
amrex::Print() << "Initializing gravity solver..." << std::endl;

Gravity<PoissonProblem> grav(&sim, phys_bc, sim.coordCenter_, HydroSystem<PoissonProblem>::density_index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here I created a Gravity<PoissonProblem> object for testing purposes. For real usage, it should all be contained within AMRSimulation/RadhydroSimulation.

src/GravityBC.hpp Outdated Show resolved Hide resolved
src/GravityBC_util.hpp Outdated Show resolved Hide resolved
// Fill data from the level below if we're not doing a solve on this level

if (level > gravity::max_solve_level) {
sim->FillCoarsePatch(level, time, grav_vector, g_old_, g_new_, 0, 3, quokka::direction::na);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we dont need g_old_ and g_new_ here anymore? This function expects integers as arguments at the 4th and 5th positions, and I think these integer arguments are 0 and 3.

@BenWibking
Copy link
Collaborator

I would recommend reading this (and the referenced Castro paper) before going much farther: #334. The algorithm is non-trivial.

@psharda
Copy link
Contributor Author

psharda commented Jan 14, 2024

I would recommend reading this (and the referenced Castro paper) before going much farther: #334. The algorithm is non-trivial.

Ah, I thought this was not going to be hard because of all the modifications to quokka in the last 2 years. If it remains non-trivial I wouldn't have the time to implement it either. Let's leave it here for now - at least a lot of the code in this PR (originally from #20 ) has caught up with the current version of quokka.

@BenWibking
Copy link
Collaborator

I would recommend reading this (and the referenced Castro paper) before going much farther: #334. The algorithm is non-trivial.

Ah, I thought this was not going to be hard because of all the modifications to quokka in the last 2 years. If it remains non-trivial I wouldn't have the time to implement it either. Let's leave it here for now - at least a lot of the code in this PR (originally from #20 ) has caught up with the current version of quokka.

It's easier, but not totally trivial.

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

There were too many comments to post at once. Showing the first 10 out of 124. Check the log or trigger a new build to see more.

src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.cpp Outdated Show resolved Hide resolved
src/Gravity.hpp Show resolved Hide resolved
src/Gravity.hpp Outdated Show resolved Hide resolved
src/Gravity.hpp 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

There were too many comments to post at once. Showing the first 10 out of 114. Check the log or trigger a new build to see more.

Comment on lines +1 to +2
#ifndef GRAVITY_HPP_
#define GRAVITY_HPP_
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: header guard does not follow preferred style [llvm-header-guard]

Suggested change
#ifndef GRAVITY_HPP_
#define GRAVITY_HPP_
#ifndef GITHUB_WORKSPACE_SRC_GRAVITY_HPP
#define GITHUB_WORKSPACE_SRC_GRAVITY_HPP

src/Gravity.hpp:467:

- #endif // GRAVITY_HPP_
+ #endif // GITHUB_WORKSPACE_SRC_GRAVITY_HPP


static const int test_solves;
static amrex::Real mass_offset;
static amrex::Real Ggravity;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'Ggravity' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

	static amrex::Real Ggravity;
                    ^

static const int test_solves;
static amrex::Real mass_offset;
static amrex::Real Ggravity;
static int stencil_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'stencil_type' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

	static int stencil_type;
            ^

@@ -0,0 +1,653 @@
//==============================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: header is missing header guard [llvm-header-guard]

Suggested change
//==============================================================================
#ifndef GITHUB_WORKSPACE_SRC_GRAVITY_IMPL_HPP
#define GITHUB_WORKSPACE_SRC_GRAVITY_IMPL_HPP
//==============================================================================

src/Gravity_impl.hpp:-1:

+ 
+ #endif


#include "Gravity.hpp"

template <typename T> Real Gravity<T>::mass_offset = 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: redefinition of 'mass_offset' [clang-diagnostic-error]

template <typename T> Real Gravity<T>::mass_offset = 0.0;
                                       ^
Additional context

src/Gravity.hpp:463: '/github/workspace/src/Gravity_impl.hpp' included multiple times, additional include site here

#include "Gravity_impl.hpp"
         ^

src/Gravity_impl.hpp:20: unguarded header; consider using #ifdef guards or #pragma once

template <typename T> Real Gravity<T>::mass_offset = 0.0;
                                       ^


#include "Gravity.hpp"

template <typename T> Real Gravity<T>::mass_offset = 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'mass_offset' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

template <typename T> Real Gravity<T>::mass_offset = 0.0;
                                       ^

#include "Gravity.hpp"

template <typename T> Real Gravity<T>::mass_offset = 0.0;
template <typename T> Real Gravity<T>::Ggravity = 0.;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: redefinition of 'Ggravity' [clang-diagnostic-error]

template <typename T> Real Gravity<T>::Ggravity = 0.;
                                       ^
Additional context

src/Gravity.hpp:463: '/github/workspace/src/Gravity_impl.hpp' included multiple times, additional include site here

#include "Gravity_impl.hpp"
         ^

src/Gravity_impl.hpp:21: unguarded header; consider using #ifdef guards or #pragma once

template <typename T> Real Gravity<T>::Ggravity = 0.;
                                       ^

#include "Gravity.hpp"

template <typename T> Real Gravity<T>::mass_offset = 0.0;
template <typename T> Real Gravity<T>::Ggravity = 0.;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'Ggravity' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

template <typename T> Real Gravity<T>::Ggravity = 0.;
                                       ^

template <typename T> Real Gravity<T>::mass_offset = 0.0;
template <typename T> Real Gravity<T>::Ggravity = 0.;

template <typename T>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: constructor does not initialize these fields: mlmg_lobc, mlmg_hibc [cppcoreguidelines-pro-type-member-init]

template <typename T>
^

template <typename T> Real Gravity<T>::Ggravity = 0.;

template <typename T>
Gravity<T>::Gravity(AMRSimulation<T> *_sim, BCRec &_phys_bc, amrex::GpuArray<Real, AMREX_SPACEDIM> &_coordCenter, int Density_,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: redefinition of 'Gravity' [clang-diagnostic-error]

Gravity<T>::Gravity(AMRSimulation<T> *_sim, BCRec &_phys_bc, amrex::GpuArray<Real, AMREX_SPACEDIM> &_coordCenter, int Density_,
            ^
Additional context

src/Gravity_impl.hpp:24: previous definition is here

Gravity<T>::Gravity(AMRSimulation<T> *_sim, BCRec &_phys_bc, amrex::GpuArray<Real, AMREX_SPACEDIM> &_coordCenter, int Density_,
            ^

@BenWibking BenWibking changed the title Gravity with subcycling [WIP] Gravity with subcycling Jan 20, 2024
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