Skip to content

Factor out mutable state from Header - #542

Open
mabruzzo wants to merge 6 commits into
cholla-hydro:devfrom
mabruzzo:factor-out-mutable-state
Open

mabruzzo wants to merge 6 commits into
cholla-hydro:devfrom
mabruzzo:factor-out-mutable-state

Conversation

@mabruzzo

@mabruzzo mabruzzo commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Overview

This PR aims to factor out some of the data members used for tracking internal state variables:

  • I created the SimRuntimeState1 class for this purpose
  • I transferred the Output_Initial, Output_Now, Output_Complete_Data, and TRANSFER_HYDRO_BOUNDARIES data members from the Header class into the SimRuntimeState class.

About Integration Counters

At the moment, I have left the integration counters (e.g. current simulation time, current timestep, elapsed wall-clock time, number of of complete cycles) in place in Header. A case could be made for transferring these quantities into the this new class, my instinct is that they are a different category of quantities (i.e. unlike the quantities inside SimRuntimeState, these quantities are ONLY modified by the top-level loop).

I suspect I'm being a bit of a perfectionist here (part of it may be laziness -- I would need to make some adjustments for recording these quantities to the HDF5 file) and could be convinced to transfer these quantities. It probably makes a sense to do this (but perhaps in a separate PR -- that change will touch a LOT of code).

Motivation

The purpose of this PR is to move us closer to having the Header class just track immutable values that don't change once the are initialized. I think this is desirable from a code-organization perspective in three (semi-related) respects:

  1. In my experience, its easier to reason about large programs (without without needing to be familiar with every aspect of the codebase) when mutable state is clearly denoted as such.

  2. In general, we want to minimize the number of these variables to the greatest extent possible. Introducing a new variable is often the easiest way to implement certain kinds of logic, because they make the control-flow much harder to follow (e.g. figuring out how the various Output_ variables worked was a bit of a chore and I've never been a fan of TRANSFER_HYDRO_BOUNDARIES). By giving these variables a dedicated home, it will make it easier for reviewers to see when a new one of these variables is introduced. Often times, it may not be necessary to introduce one of these variables at all (and instead pass the information they denote as explicit function arguments):

    • for example, I'm pretty sure it would be straight-forward to eliminate Output_Initial and it would take a little more effort to eliminate Output_Complete_Data
    • I'm also pretty sure we convert TRANSFER_HYDRO_BOUNDARIES into a dedicated argument, but this would take quite a bit more work.2
  3. (Less importantly) this is a step in the direction towards streamlining the initialization process of Grid3D. Let me explain:

    • I've never been a fan of of the way that initialization of Grid3D is so spread out. As a rule of thumb, a constructor should leave an object in a valid state, whereas we create a partially initialized Grid3D and initialize things piece-by-piece. In more (don't get me wrong, I understand why we do this)
    • For unrelated reasons, I plan to introduce a constructor for Header that fully constructs a fully initialized object directly from a ParameterMap (i.e. this will help us remove stuff from the Parameters object and it will help us pass a fully initialized Headers object onto Rad3D)
    • If the Headers object is fully initialized by the constructor (its all self-contained -- and we don't immediately start overwriting intermediate state), I think this could make it easier to follow the initialization of Grid3D.

Footnotes

  1. I could be convinced to change the name ↩

  2. I fleshed out what this might take in the data-member's docstring ↩

This branch has not been deployed

No deployments
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.

1 participant