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

Add gravity method super-cycling #381

Open
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

jobordner
Copy link
Contributor

Add super-cycled gravity, and support for super-cycling other methods.

This PR adds support for super-cycling methods, while still always satisfying method-specific timestep constraints. The purpose is to improve runtime performance, which can be especially helpful when one specific method (e.g. gravity) tends to run slower than other methods. This PR implements super-cycling for the gravity method in particular, and provides helper methods to implement super-cycling for other methods.

The time-step it uses for a super-cycled method is determined using 1. a "max_supercycle" parameter defined in the Method : <method> group, and 2. the ratio of time-steps for the super-cycled method relative to the global minimum. This allows the user to control the maximum "amount" of super-cycling, while never having to worry about violating time-step constraints.

Super-cycling works by allowing the Method to have a "solve" cycle, followed by some number k of "non-solve" cycles 0 <= k < max_supercycle. During "solve" steps, the method saves some extra fields, which are used in "non-solve" cycles to approximate the fields normally computed by the method. For the gravity method in particular, these saved fields may be either the gravitational potential, or the acceleration fields, as determined by the "Method:gravity:type_supercycle" parameter.

  • New features are documented with narrative docs: see doc/source/design/design-timestep.rst

  • Adds tests for new features: see input/test_cosmo-dd8.in

  • [ X] This is a major change or addition. Will require two reviewers.

  - reduce block dt for methods separately before computing global min
    dt in preparation for super/sub-cycling methods

Output

  - Replace mesh_color_order with more general mesh_color_scalar (untested)
  - Include index for mesh_color_scalar for arrays of scalars

Parameters

  - Replaced output_image_mesh_order parameter with
    output_image_mesh_scalar[_index] parameters

Method

  - added "ratio" Scalar<double> = index / count in MethodOrderMorton
    (for use in mesh_color_order output)
Documentation

  - updated design-timestep.rst

Field

  - added Field::values() at specified time
  - added FieldData::init_history_time() to Data::allocate()
  - changed FieldDescr::ghost_depth_ and centering_ from vector
    of pointers to vector of tuples

Parameters

  - Added Method::dt_ratio_[min|max]
  - Added Timestep parameter group
  - Added Timestep:adapt_type

EnzoMethodGravity

  - removed NEW_TIMESTEP since original code easier to read
  - added FIELD_VALUES_TIME to test Field::values() at given time
    (incomplete)
Field

  - changed Field::values(...,time) to  Field::values_at(...,time)
  - changed values_at() to template
  - updated history_time_ to include current time
  - tested and debugged values_at()

Parameters

  - changed Method : <method> :dt_ratio_[min|max] to integers
    interpret as minimum number of sub-cycles (default 1) allowed
    and maximum number of super-cycles (default 1) allowed

MethodGravity

  - added test code for Field::values_at()
  - replaced field.scale with explicit loop
Documentation
  - added a-aligned and h-aligned text
  - updated figures

Stopping
  - updated stopping criteria to support supercycling

Timestep
  - call State::init_method() in init_refine()

Parameters
  - renamed method_dt_ratio_[min|max] to method_max_[sub|super]cycle

  - removed Config() and Config(CkMigrateMessage*) initialization
    (duplicated code, defaults built into parameter access, and Config
    being depreciated anyway)

Method
  - added Method::max_[sub|super]cycle parameters and accessor methods
  - added Method::index_method_ so Method knows self index for accessing
    method-specific State attributes
State

  - Moved method parallel State::method_foo arrays into MethodState class

Method

  - BUG FIX: set_index() had int return value (with no return) not void
  - Removed Method destructor: don't want to delete schedule object
Block

  - changed Block::set_state() to virtual
  - added EnzoBlock::set_state() to update cosmology time state
  - made EnzoBlock::set_time() private
  - call private set_time_() in EnzoBlock::set_state()
State

  - Added EnzoState with redshift
  - Made State pup-able
  - Removed problematic State modifiers (EnzoState values not updated)
  - Changed Block::state_ and Simulation::state_ from raw pointer to shared_ptr

Block

  - removed unused Block::copy_()
  - prohibited Block(Block) and Block::operator =()

EnzoBlock

  - removed redshift public attribute

Tools

  - fixed valgrind-org.awk output message bug
Documentation

  - updated supercycling docs with algorithms

Compute

  - Added Method State update in Block::compute_update_method_state_()

Stopping

  - Factored out computing global and method-specific timesteps to
    Block::stopping_compute_global_dt_() and
    Block::stopping_update_method_state_()
  - Added update to State::step() and num_steps()

EnzoMethodGravity

  - Removed old debug code
Parallel

  - added [SIZE|LOAD|SAVE]_VECTOR_OBJECT_TYPE cello packing/unpacking
    macros
  - added packing/unpacking for State object
  - added State to MsgRefine
  - removed DEBUG prints
  - changed MsgRefine::face_level_ from pointer to std::vector
  - replaced manual MsgRefine packing/unpacking with macros

Adapt

  - update adapt to try to avoid supercycling (needs reworking)

Data

  - added packing/unpacking to State and MethodState objects

Mesh

  - updated Block and EnzoBlock factory methods for
    MsgRefine::face_level type change
  - moved [push|pop]_solver() from Block hpp to cpp (for debugging;
    can be safely reverted)

Fields

  - changed potential_curr and potential_prev to "temporary" fields in
    EnzoMethodGravity (may need to revert to avoid adapt issues)

Gravity

  - cleaned density_total since it looked like it could overwrite
    "density"
  - implemented a-aligned supercycling (note assumes constant dt: need
    to save time with potentials to get correct extrapolation with
    non-constant dt)

Method

  - added index() accessor to Method's index in Problem
jobordner added 10 commits March 1, 2024 12:10
Parameters

  - Added method_gravity_type_super parameter for type of super-cycling
    "potential" or "accelerations" (default)

Gravity

  - Added "EnzoMethodGravity::type_super_" attribute for extrapolation
    type super_type_[potential|accelerations]

- cleaning code: moved code blocks to functions

  - added super-cycling using saved accelerations instead of
    saved potentials

  - added acceleration_[xyz]_[prev|curr] fields

  - removed some debugging code
@jobordner jobordner added enhancement New feature or request gravity Issue/PR associated with gravity performance labels Apr 10, 2024
Data

  - have Field::dimensions() return total size m as well as mx,my,mz

Super-cycling

  - Added EnzoMethodGravity::is_supercycle_[potential|accelerations]()
  - Moved super-cycling supecific field definition, saving, and extrapolation
    from EnzoMethodGravity to general fields in parent Method
  - Reverted gravity solve to always use "potential" field as solution
@jwise77 jwise77 requested review from mabruzzo and WillHicks96 April 19, 2024 14:00

For cycles where the gravitational potential solve is not performed,
extrapolations of saved fields are used to approximate the current
fields. Which fields to be extrapolated is determined by the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fields. Which fields to be extrapolated is determined by the
fields. The fields to be extrapolated are determined by the

Comment on lines +25 to +30
set(__ARCH_C_OPT_FLAGS "-O3 -g -funroll-loops")
set(CMAKE_C_FLAGS_RELEASE "${__ARCH_C_OPT_FLAGS}")
set(CMAKE_C_FLAGS_RELWITHDEBINFO "-g ${__ARCH_C_OPT_FLAGS}")
set(CMAKE_CXX_FLAGS_RELEASE "${__ARCH_C_OPT_FLAGS}")
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-g ${__ARCH_C_OPT_FLAGS}")

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
set(__ARCH_C_OPT_FLAGS "-O3 -g -funroll-loops")
set(CMAKE_C_FLAGS_RELEASE "${__ARCH_C_OPT_FLAGS}")
set(CMAKE_C_FLAGS_RELWITHDEBINFO "-g ${__ARCH_C_OPT_FLAGS}")
set(CMAKE_CXX_FLAGS_RELEASE "${__ARCH_C_OPT_FLAGS}")
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-g ${__ARCH_C_OPT_FLAGS}")

I think these are now all unnecessary due to some more recent changes.

If you look further down on the page, we define ENZOE_C_FLIST_INIT and ENZOE_CXX_FLIST_INIT.

  set(ENZOE_C_FLIST_INIT "-Wall;-funroll-loops")
  set(ENZOE_CXX_FLIST_INIT "${ENZOE_C_FLIST}")

Currently, these add the -Wall and -funroll-loops flags. If you also want the -O3 flag instead of -O2, you should modify the list so that it reads -Wall;-funroll-loops;-O3.

As an aside, we shouldn't be adding -g as a symbol here. The canonical way to get debugger symbols is to use -DCMAKE_BUILD_TYPE=RelWithDebInfo when configuring the build-system.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't specify a build-path, Enzo-E currently defaults to a "Build type" of Release. But, I would be very happy to change the default build type to RelWithDebInfo so we always get debugger symbols in the default build (I thought this should be the default when we first shifted to CMake, but it was a debate I lost at the time).

Comment on lines +102 to +110
The gravitational potential is used to compute gravitational
accelerations, which are applied to all gravitating particles and
fields. We use a leapfrog integration scheme because it is 2nd order
accurate (small short-scale errors) and symplectic (qualitatively
correct long-scale behavior). This involves computing time-centered
accelerations. Since we start with non-time centered values, we must
either extrapolate the input (density) to the gravity solve, or
extrapolate the output (potential) from the gravity solve. In both
ENZO and Enzo-E, the extrapolation is done before the gravity solve.
Copy link
Contributor

Choose a reason for hiding this comment

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

Extrapolating the density involves the Method : pm_deposit : alpha parameter, right?

If so then our approach isn't robust in the general case. When I implemented self-gravity source terms for VL+CT, there was a whole saga where I was getting bizare looking results for a standard test problem (i.e. the "stable linear Jeans wave" that is inclined with respect to the grid and for which we can perform an analytic convergence study). Ultimately, the solution was to stop extrapolating. I suspect that we would find similar issues with PPM if we ran a similar test

In that scenario, there were no particles in that case. It's plausible that if particles dominate the short-range gravitational potential that extrapolation might be more robust.

Anyway, we probably don't want to go into detail here, but maybe we add something like the following at the end of this subsection?

.. caution:

   Some care is needed when applying extrapolation methods. For certain problems, they may not produce meaningful results.



*************************
Adaptive Time-Step Design
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't essential, but we may want to acknowledge that subcycling also exists (e.g. in Grackle and radiative transfer). Maybe just a single sentence noting that this is used as a technique on a method-by-method basis.

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

Successfully merging this pull request may close these issues.

3 participants