Conversation
The purpose of SimRuntimeState is to give mutable state variables a dedicated home (in other words, variables inside Header shouldn't get mutated once they are set).
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR aims to factor out some of the data members used for tracking internal state variables:
SimRuntimeState1 class for this purposeOutput_Initial,Output_Now,Output_Complete_Data, andTRANSFER_HYDRO_BOUNDARIESdata members from theHeaderclass into theSimRuntimeStateclass.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 insideSimRuntimeState, 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
Headerclass 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: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.
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 ofTRANSFER_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):Output_Initialand it would take a little more effort to eliminateOutput_Complete_DataTRANSFER_HYDRO_BOUNDARIESinto a dedicated argument, but this would take quite a bit more work.2(Less importantly) this is a step in the direction towards streamlining the initialization process of
Grid3D. Let me explain:Grid3Dis so spread out. As a rule of thumb, a constructor should leave an object in a valid state, whereas we create a partially initializedGrid3Dand initialize things piece-by-piece. In more (don't get me wrong, I understand why we do this)Headerthat fully constructs a fully initialized object directly from aParameterMap(i.e. this will help us remove stuff from theParametersobject and it will help us pass a fully initializedHeadersobject ontoRad3D)Headersobject 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 ofGrid3D.Footnotes
I could be convinced to change the name ↩
I fleshed out what this might take in the data-member's docstring ↩