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

EnzoProlong and MethodOutput #97

Merged
merged 58 commits into from
Jan 25, 2022
Merged

Conversation

jobordner
Copy link
Contributor

This PR implement's ENZO's SecondOrderA (et al) interpolation schemes (required for including PPM hydrodynamics in cosmological simulations) and a new scalable HDF5 data output Method, plus a number of bug fixes along the way. This is a draft pull request pending 1) merging enzo-project/enzo-e master updates 2) updating documentation and testing, and 3) adding a corresponding "scalable input" for reading HDF5 initial conditions.

Mesh
  - added Box class for computing loop indices
  - replaced loop_limits in FieldFace with Box

Cello
  - fix: changed std::numeric_limits<T>::digits to digits10

Charm
  - removed obsolete sum_long_double_[4-8]_type reductions; switched
    to n_type

Data
  - added rank parameter to FieldFace constructor for Box class,
    and since cello:rank() not available in test_FieldFace.cpp
  - removed unused FluxData methods

Enzo
  - implementd EnzoProlong to call ENZO interpolate() but with local data
    (extends coarse region by duplicating edge values)
  - changed EnzoSolverDiagonal to use temporary instead of "diagonal_D"
Cello
  - bug fix: use digits10 not digits in cello::digits_max()
Charm
  - add missing array indexing [in] to static
    EnzoBlock::NumberOfBaryonFields
Refresh
  - removed "new" from refresh-related identifiers
  - removed obsolete debugging code
Build
  - added linux_clang config
Refresh
  - Adding support for EnzoProlong's interpolation region overlapping
    multiple extra blocks
Problem
  - Added virtual Prolong::padding()
  - Removed Refresh::id_solver_
Mesh
  - Cleaning Box API
  - Added support for third "extra" Block to Box
  - Converted Box attributes to arrays
Refresh
  - Implemented most of refresh_extra_field_faces_
Mesh
  - separated Box::compute_block_start from Box::compute_region()
  - Added support for fourth "array" Block type to Box
  - set gr3_[] default to g3_
  - added Box::restrict_limits() to confine loop limits to Block
Charm
  - Added optional MsgRefresh::oname_[block|type]_ for debugging

Refresh

  - Added TRACE_COMM debugging
  - Implemented and debugged refresh_load_extra_face_ (still needs
    field value copies)
  - Implemented refresh_extra_send_()

Mesh
  - added Block::padded_face_array_[] to store padded arrays
  - added Block::padded_face_array()
  - added Block::padded_face_array_[de]allocate()
  - added Block::name(Index) to get name of neighboring blocks

Data
  - added DataMsg::padded_face_
  - added DataMsg::padded_face_field_list_
  - added DataMsg::set_padded_face()
  - added padded_face_field_list_

Enzo
  - added missing array copy for 3D problems

Tools
  - changed obsolete xrange to range in plot_mesh.py
Testing

  - removed "dump" from Output in PPML tests which was failing in
    random queueing

Debugging

  - added temporary cello::sum() and cello::copy() used in
    enzo_prolong debugging

  - added cello::hex_string() to create pseudo-random hex string for
    MsgRefresh::tag_ and DataMsg::tag_ for debugging messages

  - updated debugging code in MsgRefresh
  - updated debugging code in control_refresh
  - updated debugging code in DataMsg

EnzoProlong

  - added Block::refresh_extra_apply_ to call EnzoProlong only after
    all expected face data have been received

  - added refresh_type parameter to refresh_load_extra_face_

Mesh

  - Updated Box to not overwrite receive block with extra since needed
    for EnzoProlong array receive
  - Added Box_type to Box class for
  - Added Box::get_region_size()
  - Added rank,level,face,child to Box constructor instead of separate
    set_face(), set_child(), etc.

Method

  - fixed timestepping in EnzoMethodGravity to match ENZO (had grouping error)
  - removed const from virtual double Method::timestep() required by change
    to ENzoMethodGravity::timestep()

Data
  - added ENABLE_PADDING to FieldFace
  - Moved padded_face_array_ from Data to FieldData::padded_array_
  - added padded_array_dimensions_[]
  - added padded_array_fields_[]
  - removed FieldFace::box_ since doesn't need to be stored
  - cleaned padded array code in
    FieldFace::[array|face]_to_[array|face]()
  - added code to FieldFace to copy send-recv face zones to padded
    array in FieldFace::[array|face]_to_[array|face]()
  - fixed typo c3[3] = {3} in FieldFace::face_to_face()

Messages
  - updated padded_array attributes in DataMsg

ENZO

  - added int() around sign() calls in some fortran code (calcdiss)
Build
  - removed obsolete ncsa-bw check in build.sh that points to user
    home directory

Testing
  - added test_Box (empty)
  - updated test_Field for coarse fields

Debugging
  - removed control_refresh.cpp TRACE_COMM debugging
  - updated debugging in DataMsg
  - cleaned FieldFace debugging
  - added temporary ENABLE_EXTRA to control_refresh.cpp
  - added MsgRefresh::set_block_[name|type]

Mesh
  - cleaning Box API
Prolong
  - converting face padded_arrays to single coarse field per block
    (incomplete)
Data
  - added FieldData::coarse_values()
  - added FieldData::[de]allocate_coarse()
  - added FieldData::coarse_dimensions()
Documentation

  - formatting

Debugging

  - updated MsgRefresh.cpp, control_refresh.cpp
  - cleaned DataMsg

EnzoProlong

  - renamed refresh_extra_*() as refresh_coarse_*()
  - updated call to refresh_load_coarse_face_()
  - updated refresh_load_coarse_face_() parameters
  - added main S->r send for padded prolong

Mesh

  - refactored Box API: split get_limits() into get_start_stop() and
    get_start_size()
  - refactored Box to clarify role of get_start_*() parameters
    (first defines block to intersect region, second to define
     index coordinate system)
  - added Box::coarse_size_[] and coarse_ghost_[] initialized by
    set_coarse()
  - bugfix in FieldData::coarse_dimensions() for odd ghost depths

Charm

  - renamed and updated DataMsg::set_padded_face() as
    set_coarse_array()
  - updated DataMsg coarse_array parameters

Prolong

  - renamed Prolong::padding() as Prolong::coarse_padding()
  - removed padding code from FieldFace since handled by
    FieldData::coarse_array_
  - removed unused Prolong::monotonic_ and ::positive_
  - changed Prolong::apply() to void return
  - added virtual Prolong::array_sizes_valid (true default)

Fortran

  - removed obsolete [DEBUG_ERROR_WARNING]_MESSAGE defines
Cello

  - cleaning macros for sizing, saving, loading message data

  - updated random number generator in cello::hex_string() to not
    be the same on different processors

Refresh

  - removed redundant Box method calls
  - fixed interpolation factor: add not average
  - finished implementing Block::refresh_coarse_apply_()

Data

  - renamed DataMsg::field_[array|data}_ as field_[array|data]_u_ to indicate union
  - replaced manual size/load/save with cello.hpp macros
  - fixed data size check ASSERT call position
  - updated print() to include tag_
  - added Box BlockType::none for not intersecting region

EnzoProlong

  - finished implementing

Tools

  - updated valgrind to include additional issues in generated valgrind.org file
Problem

  - added MethodRefresh to manually add refresh operations between other Methods

Refresh

  - added debug to control_refresh.cpp
  - fixed logic for calling refresh_load_[field|coarse]_face_ and corresponding counters
    (note count expected receives not number sent)
  - fixed BlockType argument to box_sr.get_start_stop for (iam3,iap3) indices
  - fixed BlockType arguments to box_[er|se].get_start_stop() for (ifms3,ifps3) indices
  - fixed rr scaling to be num_children() only when levels actually differ
  - fixed loop limits in DataMsg::update()
  - fixed (re)setting pad = 0 in else of if (refresh_fine) in FieldFace face_to_array()
    array_to_face()
  - simplified calls to prolong->apply in FieldFace::[array|field]_to_field()
  - adding Refresh::set_min_face_rank() and ::set_ghost_depth(), ::sync() (not called yet)
  - added return_bypass to return_enum for Blocks that have nothing to do in a Solver

Mesh

  - split BlockType::coarse into BlockType::coarse_receive and
    BlockType::coarse_extra

Config

  - added MethodRefresh parameters to Config
    - method_refresh_field_list;
    - method_refresh_particle_list;
    - method_refresh_ghost_depth;
    - method_refresh_min_face_rank;
    - method_refresh_all_fields;
    - method_refresh_all_particles;
  - made Config::index_schedule public for consistency

Enzo

  - adding debugging to EnzoSolverBiCgStab
  - incorporating ENZO Fortran code error handling into EnzoMethodPpm to
    exit immediately on errors
  - adding ENZO errors
  - added missing ierror parameter to flux_[hll|twoshock] argument
    lists (was in call)
  - added returns after errors in [xyz]euler_sweeps to exit with error earlier
    and not try to "compute" with nan's
Testing

  - fixed B_COPY -> B_copy in test_cosmo.incl
  - implemented test_Box.cpp (no checks, currently for comparing
    with expected values by hand)
  - updated test_FieldFace for change to FieldFace::set_ghost() arguments
    (int instead of bool)
  - add src/Enzo/test_interpolate.F for testing setup for calling
    interpolate.F (old but hadn't added to git)

Mesh

  - changed Block::create_face() arguments from logical lg3 to values g3
  - added lpad argument to Box::get_start_[stop|size]() whether to
    include padding (need false for fine blocks true for coarse)
  - removed temporary set_padding(0) in refresh_coarse_apply_
    (not needed with lpad argument)
  - moved box set_padding() inside set_box_()
  - fixed coarse loop indices in array_to_face()
  - update FieldFace::ghost_ from bool to int for size
  - fixed logic for computing pad (0 if not refresh_fine)
  - changed Box to add padding when get_start_[stop|size] called not
    when computing region (since not always used)
  - added Box::apply_padding_()

Method

  - renamed i_f as index_field in MethodDebug
  - updated EnzoSolverDd::pack_field_() for including ghosts only when
    refresh_type == refresh_fine
  - added EXIT_ON_ERROR to SolveHydroEquations (EnzoMethodPpm)
  - removed obsolete setting ierror to constants in [yz]euler_sweep.F

Refresh

  - BUG FIX: fixed use of i_f index instead of index_field in
    refresh_coarse_apply_
  - fixed FieldData::coarse dimensions() for non-centered fields
    (cx was used instead of cy)
Input

  - removed prolong parameters for testing
  - added input/schedule_time_0.01.incl
  - removed old/unused fields from test_cosmo.incl

Testing

  - removed mesh-balanced test since redundant with adapt_L5
  - run adapt_L5 regression test to 0.05 instead of 0.1 to speed up regression
  - changed gravity solver tolerance from 0.01 to 0.1 to speed up regression
  - updated test/index.php for above changes
  - cleaning: removed commented-out tests

Prolong

  - readded TRACE_PROLONG output to Prolong objects for verifying type

Parameters

  - removed obsolete interpolation_method parameter
Debug

  - consolidating debug macros in cello.hpp
  - removing debug code from control_refresh.cpp

Refresh

  - incorporate refresh->field_list_dst() as well as field_list_src()
    for padded prolong refresh
  - duplicated Box object in refresh_coarse_apply_() that was
    used for two separate index computations
  - moved FieldFace::accumulate_() into Refresh::accumulate()
  - Added Refresh::box_accumulate_adjust()

Output

  - added use_min_max parameter to OutputImage()
  - fixed OutputImage pup()--added missing attributes
  - changed log() in OutputImage to log(fabs())

Prolong

  - consolidated duplicated code in ProlongLinear::apply_() for
    accumulate = 0 and 1
  - added support for accumulate in EnzoProlong

Method

  - removed non-accumulated fields from ir_post refresh in EnzoMethodGravity
    (accelerations and density)
  - added DEBUG_COPY_DENSITIES to EnzoMethodGravity
  - removed errant debug code from EnzoMethodPmDeposit

Testing

  - removed obsolete collapse tests test_method_gravity_cg-[18].unit
  - Cleaning Sync, preparing to add error checking

Initial

  - Moved initialize_[prolong|restrict] before initialize_[method|solver]
    since initialize_[prolong|restrict] create "default" prolong/restrict
    which must be first

Prolong

  - moved coarse_padding() from Prolong to Refresh, so can set to 0
    if refresh accumulate_ is true
  - Made Prolong::coarse_padding() protected but friend to
    Refresh::coarse_padding()

Problem

  - changed Problem::[prolong|restrict]_ to lists [prolong|restrict]_list_
  - changed all NULL's to nullptr's in problem_Problem.?pp
  - changed initialize_[prolong|restrict]_ to create default prolong/restrict
    check that it's creating the first prolong/restrict objects in
    the lists
  - changed Problem::[prolong|restrict]() methods to accept optional argument
    i, with default 0 to return default first in list
  - added "use_linear" argument to EnzoProlong constructor

Refresh

  - adding prolong and restrict to Refresh

Testing

  - cleaning test_Sync

Parameters

  - added Prolong : enzo : use_linear parameter for debugging
    (EnzoProlong::apply() calls ProlongLinear::apply())

Solvers

  - Changed restrict/prolong arguments to EnzoSolver[Dd|Mg0] to
    index_prolong/index_restrict
  - adding debugging to Sync to test for over-counting (INCOMPLETE)
  - moved [mul|div]_by_density() from FieldFace class to cello:: namespace

Debug

  - removed tag_ attribute and other debugging code from MsgRefresh
  - and DataMsg addressed some minor compiler warning messages (-Wall)
  - add checks when copying fields for debugging in EnzoMethodGravity
  - and EnzoMethodPpm

Rferesh

  - Added index_[prolong|refresh] to Refresh
  - Removed Prolong / Restrict from FieldFace (use from Refresh instead)

Parameters

  - Changed default Field:prolong from "linear" to "enzo"

Solver

  - Added prolong/restrict arguments to Solvers
  - changed EnzoSolverBiCgStab::res_tol_ from long double to double
    (suspect this was causing compiler issues with optimization)
Control

  - bug fix: Moved deallocating fluxes from MethodFluxCorrect.cpp to
    control_compute.cpp to avoid memory leak if MethodFluxCorrect isn't
    called

Debug

  - Updating MethodDebug for added parameters (incomplete)
  - Added parameters to "debug" method: "print", "coarse", "ghost"
    (incomplete)
  - added debug code for copying fields in EnzoSolverBiCgStab

Parameters

  - Added method_refresh_prolong

Method

  - EnzoMethodGravity updates
    - Added index_prolong parameter to EnzoMethodGravity
    - Re-added accelerations to ir_post refresh in EnzoMethodGravity
    - fixed typo: naming ir_exit_ refresh

Solver

  - EnzoSolverBiCgStab can produce poor solutions at mesh level jumps
    if restart != 1: resetting to 1 with WARNING message to bypass
    until this is addressed

Cleaning

  - Removed obsolete EnzoMethodCheckGravity
  - Removed obsolete files in tools directory
  - Added tools/index.org file to document remaining available tools
Mesh

  - Added BlockTrace class to aid traversing distributed
    array-of-octrees partition (range of elements in root-block array)
  - Added [pr]_method_output_<foo>() entry methods for MethodOutput

Parameters

  - Added initial_hdf5_* parameters for InitialHdf5
  - Renamed method_refresh_<foo> to method_<foo>
  - Added method_output_blocking parameter to define scope of
    domain for each output file

Testing

  - Added test_BlockTrace unit tests
  - Removed unused code from test/index.php

Method / Output

  - Added MethodOutput "method" for writing HDF5 output

Cello

  - fixed LOAD_ARRAY_TYPE()

Charm

  - Added MsgOutput Charm++ message for use by MethodOutput
  - cleaned MsgRefresh: removed debug code

Initial

  - Added InitialHdf5 stub

Cleaning

  - addressed some -Wall compiler warnings
Build

  - removed obsolete "new_output" from SConstruct

Output

  - Removed unused index parameter from Io class hierarchy
  - removed unused Io::data_count_
  - Copied expand_name() and directory() from Output to Cello so
    can use in MethodOutput
  - removed NEW_OUTPUT from control_output.cpp

Mesh

  - Added index_home to BlockTrace (not necessarily the same
    as index_root)
  - Added data_size, [save|load]_data to Index

Cello

  - Added string, object, and object_ptr to size/load/save type macros

Charm

  - Updating MsgOutput for block name, file pointer, and tag (for
    debugging)
  - Converting manual size/load/save in MsgOutput::[un]pack() to
    use cello.hpp macros

Parameters

  - Added method_output_[file|path]_name parameters
  - Fixed method_field_list to be list of string not integer

Method

  - Implementing MethodOutput (sequencing works, file created and closed,
    need to add HDF5 write calls)
  - Added MethodOutput ScalarData for file counter (needed per-block
    since may have multiple Blocks writing files on same pe
  - Updated MethodRefresh field and particle lists to be
    string not int

Simulation

  - Removed Simulation sync objects for obsolete "new_output"
Input

  - Removed flux_correct from test_double_mach to avoid crashing

Charm

  - Added IoBlock, Block lower/upper to MsgOutput

Problem

  - Added Factory argument to Problem::initialize_method()
    (required for MethodOutput)
  - Made argument orderings in Problem::create_[*]() more consistent

Output

  - Added data_size() and [load|save]_data() to Io class hierarchy
    for including in MsgOutput
  - Made Io class hierarchy pup-able
  - Updated Io classes to store metadata and not access via
    Block / Hierarchy classes (since IoBlock may be accessed
    on different Block than where created in MethodOutput)
  - Finished Block and field data output in MethodOutput
    (particle data still not implemented) (untested at scale)

Mesh

  - Added Index::operator[]
  - Added virtual Block::factory() for use by MethodOutput()

Tools

  - Made plot_mesh.py more forgiving, supporting any text file
    containing "words" that look like Block names B0:0_1:00
Method

  - removed debugging code from MethodOutput
  - added support to output particle data
Output

  - bug fix: check for any fields or particles in MethodOutput
    lists before setting in DataMsg
  - bug fix: added missing HDF5 file close to MethodOutput
  - added "writing data file" monitor output to MethodOutput
  - Cleaning and optimizing OutputImage (incomplete)
  - Added trace_memory debugging to OutputImage

Testing

  - added new-output to test-adapt-L5-P1 regression test
  - fixed typo in test/index.php

Parameters

  - removed image_block_size parameter
  - replaced image_block_size with image_size in input files
  - changed image_size default from [0,0] to [512,512]

Documentation

  - marked image_block_size parameter as depreciated

Data

  - added FieldFace::ghost() (for debugging MethodImage)

Tools

  - Tweaked valgrind-org to check src not source for source files

Cleaning

  - Removed unused file control_Control.hpp
  - Spell-checked comment in control_stopping.cpp
@jobordner jobordner self-assigned this May 14, 2021
Checkpoint

  - Adding MethodCheckpoint (incomplete)
  - Added Simulation_r_write_checkpoint_[output|method]
  - Added Main::p_[output|method]_checkpoint

Data

  - bug fix: removede delete particle_data_ in update
    (deleted in destructor if needed)

Parameters

  - renamed parameters method_output_[file|path]_name as
    method_[file|path]_name

Initial

  - moved InitialHdf5 to EnzoInitialHdf5
  - removed obsolete EnzoMethodHydro
  - fixed array initializers in EnzoInitialMusic

Config

  - Split EnzoConfig::read() into subfunctions
@mabruzzo mabruzzo linked an issue Aug 9, 2021 that may be closed by this pull request
Build

  - added "new_adapt" variable to define NEW_ADAPT
  - added default for CELLO_VAR in config files

Adapt

  - Added quiescence detection (QD) after doneInserting() call
  - Added p_adapt_update() entry methods (Main and Block) for
    additional QD
  - Changed Block::adapt_ to local variable

Checkpoint

  - Added single-thread to CkStartCheckpoint (should be generalized
    with parameter)

Cleaning

  - Removed unused r_adapt_*() entry methods

Monitor

  - Added NEW_ADAPT to Monitor::header()

Testing

  - Fixed Balance tests to include parallel_arg
Parameters

  - Added Method:type parameter to allow multiple instances of a
    method but with different parameters.  E.g. "output" method with
    different schedules, blocking, files or particle lists, etc.
    Uses 'list' identifier if none for backward compatibility.
@jobordner
Copy link
Contributor Author

@mabruzzo Hi Matt, shall I pull the merges with enzo-project/main (new-output-merge), or would it disrupt any reviewing in progress? Alternatively, I could open a separate PR with new-output-merge. I used git-imerge to resolve conflicts, which generated a lot of commits ("This branch [new-output-merge] is 950 commits ahead of enzo-project:main", versus new-output which involves "only" 47 commits ahead of main when it was forked), though I expect each commit should be relatively self-contained. A separate PR would also let John @fearmayo review the merged version since he's already approved the unmerged one.

@mabruzzo
Copy link
Contributor

mabruzzo commented Sep 3, 2021

@mabruzzo Hi Matt, shall I pull the merges with enzo-project/main (new-output-merge), or would it disrupt any reviewing in progress? Alternatively, I could open a separate PR with new-output-merge. I used git-imerge to resolve conflicts, which generated a lot of commits ("This branch [new-output-merge] is 950 commits ahead of enzo-project:main", versus new-output which involves "only" 47 commits ahead of main when it was forked), though I expect each commit should be relatively self-contained. A separate PR would also let John @fearmayo review the merged version since he's already approved the unmerged one.

Hi @jobordner. Sorry for not more actively reviewing this. (I intend to come back to this on Tuesday). I've been a little distracted with some research. It would actually be great if you could pull the merges with enzo-project/main into this branch (it would make the "file diff" a lot easier to understand). Making a separate PR also works (I don't have any preference).

@jobordner
Copy link
Contributor Author

Update: I've had to work on another project for a bit, but want to try to finish this up. I'm working on re-merging with main to reduce the number of commits before updating this branch. (Note that since others are developing off of this branch, rebasing wouldn't be a good idea.)

@mabruzzo
Copy link
Contributor

Good to know. I actually started reviewing this a couple of days ago, but I didn't get particularly far. Do you think it would make sense for me to hold off on continuing my review until after you have updated this PR? (In some sense, this would make things easier for me).

This is probably obvious to you, but I just wanted to point out that one hypothetical option for reducing the number of intermediate merging commits is squashing them together (or consolidating the changes via soft reset).

@jobordner
Copy link
Contributor Author

I figured dealing with conflicts from merging < 50 actual commits would be less work than working through conflicts from squashing > 900 application-generated commits, so I just remerged manually. test_restart_vlct.unit still fails due to non-centered fields not being refreshed correctly, which I'll work on this week.

Copy link
Contributor

@mabruzzo mabruzzo left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to review this. Over the past week, I tried to really dig into this. Overall, I think this looks great!

Overall, I dug a lot deeper into the new Input and Output machinery than the updates to EnzoProlong. I know I'm stating the obvious, but in the future it would be helpful if the PRs could be a little more focused (I'm also definitely guilty of making massive PRs too).

The vast majority of my review-comments are clarifying questions or requests for you to add comments to the code. I think that charm++'s reactive nature makes some of the control flow difficult to follow in MethodOutput::compute and in the refresh machinery. I found the latter especially difficult to follow (primarily because there are a lot more moving parts and I'm unfamiliar with most of them). If you could add in some of the high-level comments that I requested, I think that could make the code a lot easier to follow.

I also recommended a bunch of very minor whitespace changes, but definitely feel free to ignore these. (If you aren't opposed to making these changes, it might be easiest for you to batch all of these together through Github).

General Comments/Questions:

  • We seem to be missing documentation about the parameters for InitialHdf5, MethodOutput, MethodRefresh. I don't want this to block the merging of this PR, but I do think that you should at least create an issue to keep track of what is undocumented.
  • would you consider adding block->inital_done(); calls to the initializers introduced with VL+CT?
  • Does MethodOutput write a list of the output files, anywhere? (Equivalent to what is stored in the .block_list or .file_list files by the old output approach). Or is the intention to have users infer the output files based on values from the parameter file?
  • The test/SConscript file needs a little attention. A lot of commented-out tests have been added to this file and they don't seem to be properly organized. I assume that this occured during the merge.

I just want to reiterate that I think these changes looks fantastic! Great work!

P.S. If I have time, I might try re-running my Grackle tests again just to confirm that it still works in SMP-mode. But, as I recall, this branch worked correctly so this shouldn't really be any kind of blocker on merging.

src/Cello/cello.hpp Outdated Show resolved Hide resolved
src/Cello/cello.hpp Outdated Show resolved Hide resolved
src/Enzo/enzo_EnzoMethodGravity.hpp Outdated Show resolved Hide resolved
src/Enzo/enzo_EnzoSolverCg.hpp Outdated Show resolved Hide resolved
src/Enzo/enzo_EnzoSolverDiagonal.hpp Outdated Show resolved Hide resolved
Comment on lines +74 to +75
id_prolong_(0),
id_restrict_(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

int g4[] = {4,4,0};
int g0[] = {0,0,0};
int g1[] = {1,1,0};
// linear / enzo
Copy link
Contributor

Choose a reason for hiding this comment

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

when you say linear/enzo are you talking about prolongation without padding vs prolongation with padding?

const int nx = mx - 2*gx;
const int ny = my - 2*gy;
const int nz = mz - 2*gz;
enzo_float * flux_array = (single_flux_array) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining what you are doing here?

Comment on lines +860 to +861
method_flux_correct_single_array =
p->value_logical (full_name + ":single_array",true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you add documentation for this parameter?

if (pmx) (*pmx) = mx;
if (pmy) (*pmy) = my;
if (pmz) (*pmz) = mz;
return (mx*my*mz);
}

/// Copy flux values from an array to the FluxFaces flux
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
/// Copy flux values from an array to the FluxFaces flux
/// Copy flux values from an array to the FaceFluxes flux

jobordner and others added 5 commits September 27, 2021 17:21
Co-authored-by: Matthew Abruzzo <matthewabruzzo@gmail.com>
Co-authored-by: Matthew Abruzzo <matthewabruzzo@gmail.com>
Co-authored-by: Matthew Abruzzo <matthewabruzzo@gmail.com>
@mabruzzo
Copy link
Contributor

mabruzzo commented Dec 5, 2021

I was recently reminded of the two failing VL+CT tests (while I was trying to do some development off of a branch of this). I had completely forgotten about that problem (and forgot to mention that in my review).

James, I just wanted to remind you of this issue (in case you forgot about it too), since I think this is arguably more important than my other suggestions.

Data

  - removed ASSERT that was crashing on non-centered (odd-length)
    field data
  - Set Box centering in FieldFace face_to_array(), array_to_face(),
    and face_to_face()
  - Adjust index_[min|max] in Box for non-centered fields
    (centering_[i] == 1)

Testing

  - Fixed FieldDescr and FieldData tests
@jobordner
Copy link
Contributor Author

Thanks for the reminder! I've addressed in commit 2684f31 "Updating Box class to handle non-centered fields", and the VL+CT tests now pass.

@jobordner
Copy link
Contributor Author

Just to quickly address your high-level comments...

  • We seem to be missing documentation about the parameters for InitialHdf5, MethodOutput, MethodRefresh. I don't want this to block the merging of this PR, but I do think that you should at least create an issue to keep track of what is undocumented.

I've started a separate review of parameters to check consistency between documentation and code, so I think that can be a separate PR.

  • would you consider adding block->inital_done(); calls to the initializers introduced with VL+CT?

I believe all "initial_done()" calls have been added, but I'll double-check.

  • Does MethodOutput write a list of the output files, anywhere? (Equivalent to what is stored in the .block_list or .file_list files by the old output approach). Or is the intention to have users infer the output files based on values from the parameter file?

I took that out because it was buggy and I figured they could be generated later using e.g. h5ls, but I found in practice generating them on Frontera took many hours. I'll address this in my next PR on "new-input" since that deals with I/O.

  • The test/SConscript file needs a little attention. A lot of commented-out tests have been added to this file and they don't seem to be properly organized. I assume that this occured during the merge.

The new cmake / ctest infrastructure should make this obsolete.

Copy link
Contributor

@mabruzzo mabruzzo left a comment

Choose a reason for hiding this comment

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

I believe all "initial_done()" calls have been added, but I'll double-check.

You're right. You did do that.

This all sounds sensible to me. Approving and merging into main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants