Streamline incons #1590
Closed
Streamline incons #1590
Conversation
This file contains 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
added 3 commits
October 29, 2024 18:01
Adds "Instant" counterparts to Clock and VariableClock, which report absolute times as Java instants, but still step time according to an Aerie duration. Also adds some minimal interoperability between absolute and relative clocks, and some useful derivations including comparisons. Using these, also adds a global Instant-based clock to Resources, along with exposing both the duration- and instant-based clocks as resources. In doing so, we add a new parameter for the plan's start time to Resources.init(). This in turn required refactoring some unit tests, and I took the opportunity to clean up the test construction a little bit as well. This revealed a way to correctly initialize Resources, i.e., to call Resources.init() exactly once per initialization of each test model. As a result, I refactored the clock handling to remove the awkward reinitialization pattern, since that pattern was added to handle test code.
Adds the InconBehavior interface and some of the surrounding machinery to connect it. The thrust of this idea is to wire together incon reading, fincon writing, and cell creation, such that the easiest way for a modeler to create a new cell is to correctly wire up the incons. This means that wiring up those incons should be relatively painless, it should minimize duplicating necessary information, it should eliminate specifying any unnecessary information, and side-stepping the incons process should require rebuilding a large portion of the resource stack, as a strong discouragement to doing so. This commit does not compile. The status of various classes is: 1. I'm mostly happy with InconBehavior itself. It's a neatly packaged bit of behavior, it composes well (see InconBehavior.map), and I think it's legible as-is. 2. I may want to remove InconBehaviors static constructors "constant" and "serialized". I think those belong in MutableResource, as part of the "notSaving" and "serializing" helper methods instead. 3. I'm not happy with MutableResource yet, as we're now duplicating information for the incon behavior (name and ValueMapper) that we'll give later if/when we register it. I'm wondering if we should roll registration into this method as well, perhaps with a flag or alternate constructor to not register a resource... 4. Everywhere we build MutableResources needs to be updated, pending what we decide to do for the step above. Additionally, we'll need to document how this changes the MutableResource interface and include that in the migration notes should this be adopted by Aerie.
added 2 commits
October 30, 2024 11:15
This commit and the one before it attempt to coalesce building a resource and registering it, since you need similar information for both operations. I really like one thing about this approach: Discrete primitive resources get value mappers by default, meaning they "just work" with little to no additional configuration. There are a few things I don't like about this approach: 1. Writing the builder is tedious and repetitive, since I'm writing one for each kind of resource (discrete, Polynomial, clock, etc.). It strongly feels like most of this behavior could be moved up to a higher level of abstraction, somehow. 2. This makes registration of MutableResource quite different from registration of "regular" Resources. Relatedly, specialized MutableResource constructors now need to either use the builder or return the builder, both of which feel wrong. You're either leaking an abstraction that you likely shouldn't be, or you're restricting the modeler in an awkward, artificial way by making all the builder choices for them. Or, you end up bloating the special-purpose constructors too much to expose all the builders' functionality. 3. Relatedly, for things like Polynomial resources, where we often register an approximation, it feels like we should handle that akin to discrete. But we can't, strictly, since what we register and what we return are different. Building the choice of approximation into this class feels wrong. It suggests that if you build the way to register into the resource construction, it ought to be general enough to apply to any resource, not just polynomial / discrete. 3. Passing the registrar to the builder now feels awkward. I think maybe the registrar should become a singleton we access at-will, like the Logger. Alternate designs include: 1. Making a general resource builder, and letting particular kinds either sub-class that builder or write helper methods that fit the arguments of that builder. For example, the general builder accepts a ValueMapper<Dynamics>, and discrete resources could offer a function from ValueMapper<T> to ValueMapper<Discrete<T>>. The awkward bit here (maybe?) is registration, as most resources can't be registered. Only discrete and linear can be. However, point 3 above suggests if we expand our notion of registration to include a mapping step, maybe anything could be registered, they just aren't by default... 2. Get rid of the builder altogether, and instead lean on that "helpers that build the arguments of your general constructor" pattern *hard*. This is what we had in streamline up to this commit, and it works well without tons of boilerplate code to maintain. Maybe we could just make the registrar smarter, such that it can at least pull the name out of Naming. For more consistency, we could even remove the regsitrar constructor that takes a name, and ask users to directly invoke it like `register(name(r, "newName"), ...)` if they want to change the name. If we really wanted to lean even further in that direction, we could register the value mapper for discrete stuff like we register the name, in a third-party class like Naming, and have the registrar pull it from there. Then you'd say something like `register(serializable(name(r, "newName"), mapper))` to get the full behavior. 3. Maybe we could return InconBehavior<MutableResource<...>> instead of just the resource itself? If you do that, I think you could at least push the choice of how to save the resource out of the constructors, and make it a post-step where you choose notSaved or serialized. Then if you want to share information between that choice and registration, maybe DiscreteResources could offer helper methods for the most reasonable combinations? I.e., something that chooses serialized + registered with the same information? It still feels different from other kinds of registration in an awkward way... After working through these options, I'm leaning towards option 1, keep a resource builder as a place to centralize saving and registration together, but lift it to MutableResource in general. Then, methods like DiscreteResources.discreteResource could instantiate a builder with more default options chosen, but still expose the full generality. With a little care, they might even be able to return a sub-class of the general builder, with additional convenience methods exposed...
DavidLegg
force-pushed
the
feature/streamline-incons
branch
from
d9a38ad
to
959b577
Compare
|
Meant to open this in my own repo, sorry! |
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.
DO NOT MERGE - this PR is for displaying this prototype code only, and is not meant to be merged.
This is a prototype of adding incon/fincon machinery to streamline, rather than writing it into core. Credit to Alex Greer for a lot of the ideas in here, especially the idea that streamline's particular implementation of a cell would be a good place to mix in incon/fincon functionality.
Since the "read incon" behavior and "write fincon" behavior should be tightly coupled, I've created an InconBehavior interface to package them together. The signatures on these methods allow them to be extended (see InconBehavior.map), which is important for allowing users to write an InconBehavior for simple types, that we can automatically extend to more complex or "wrapped" types. Otherwise, the InconBehavior is left highly general, treating incons as String-SerializedValue key-value pairs, which can be freely queried for incons and freely written for fincons.
The key principle in this design is "Every cell has an incon behavior." There are some cells where that behavior is degenerate, i.e., "For incons, use this constant value, and for fincons, write nothing out." However, this is an opt-out system, where the modeler needs to choose not to save the cell's value, rather than needing to remember everywhere else that they should save the cell's value.
To that end, I've modified streamline's cell allocation function to take an InconBehavior instead of an initial value. This interacts with a static incon manager to retrieve the initial value for the cell and register the fincon behavior, simultaneously. Doing the two operations simultaneously, I hope, will contribute to keeping the two halves of this behavior tightly coupled.
On top of this, I'm modifying the MutableResource constructors to add more user-friendly support for common use cases, namely
There will still be a more general constructor for MutableResource which allows any incon/fincon behavior, to support edge cases.
Finally, that static incon manager is initialized and loaded with a set of incons when building the registrar, since building the registrar has evolved into the natural place to initialize all the streamline singletons, like the simulation clock and the global logger. That static incon manager then exposes a method to write a fincon, which the model may call at any time to invoke all the registered fincon behaviors, compose a full system fincon, and save that fincon. Note that this will mean it's up to the mission model to define something like a simulation config parameter and model task, or a "Fincon" activity, to actually save the fincon.