From 959b577911dfe54dab3845339859a27444f0b859 Mon Sep 17 00:00:00 2001 From: David Legg Date: Wed, 30 Oct 2024 14:22:26 -0700 Subject: [PATCH] WIP - Adding resource builders 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, and discrete resources could offer a function from ValueMapper to ValueMapper>. 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> 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... --- .../streamline/core/MutableResource.java | 2 +- .../modeling/clocks/ClockResources.java | 9 -- .../clocks/InstantClockResources.java | 7 - .../modeling/discrete/DiscreteResources.java | 132 ++++++++++++------ .../polynomial/PolynomialResources.java | 78 ++++++++++- 5 files changed, 165 insertions(+), 63 deletions(-) diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/MutableResource.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/MutableResource.java index 946e918d54..d05c7e053c 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/MutableResource.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/MutableResource.java @@ -88,7 +88,7 @@ static InconBehavior>> serializing(String key, D d return serializing(key, pure(defaultValue), standardDynamicsMapper(mapper)); } - private static ValueMapper>> standardDynamicsMapper(ValueMapper baseMapper) { + static ValueMapper>> standardDynamicsMapper(ValueMapper baseMapper) { return new ValueMapper<>() { @Override public ValueSchema getValueSchema() { diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/clocks/ClockResources.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/clocks/ClockResources.java index f8ada92031..a02bf502b3 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/clocks/ClockResources.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/clocks/ClockResources.java @@ -7,25 +7,16 @@ import gov.nasa.jpl.aerie.merlin.protocol.types.Duration; import static gov.nasa.jpl.aerie.contrib.streamline.core.Expiring.*; -import static gov.nasa.jpl.aerie.contrib.streamline.core.MutableResource.*; import static gov.nasa.jpl.aerie.contrib.streamline.core.Resources.signalling; import static gov.nasa.jpl.aerie.contrib.streamline.core.monads.ResourceMonad.bind; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.Discrete.discrete; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.DiscreteResources.not; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.monads.DiscreteResourceMonad.map; import static gov.nasa.jpl.aerie.merlin.protocol.types.Duration.EPSILON; -import static gov.nasa.jpl.aerie.merlin.protocol.types.Duration.ZERO; public final class ClockResources { private ClockResources() {} - /** - * Create a clock starting at zero time. - */ - public static Resource clock(String name) { - return resource(serializing(name, Clock.clock(ZERO), null /* TODO - get auto value mapper for Clock type */)); - } - public static Resource> lessThan(Resource clock, Resource> threshold) { return signalling(bind(clock, threshold, (Clock c, Discrete t) -> { final Duration crossoverTime = t.extract().minus(c.extract()); diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/clocks/InstantClockResources.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/clocks/InstantClockResources.java index 204ef2c3c2..18f696f657 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/clocks/InstantClockResources.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/clocks/InstantClockResources.java @@ -13,13 +13,6 @@ import static gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.DiscreteResources.constant; public class InstantClockResources { - /** - * Create an absolute clock that starts now at the given start time. - */ - public static MutableResource absoluteClock(Instant startTime) { - return resource(new InstantClock(startTime)); - } - public static Resource addToInstant(Instant zeroTime, Resource relativeClock) { return addToInstant(constant(zeroTime), relativeClock); } diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/discrete/DiscreteResources.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/discrete/DiscreteResources.java index 1da703fc28..51cbaee516 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/discrete/DiscreteResources.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/discrete/DiscreteResources.java @@ -2,6 +2,7 @@ import gov.nasa.jpl.aerie.contrib.streamline.core.*; import gov.nasa.jpl.aerie.contrib.streamline.core.CellRefV2.CommutativityTestInput; +import gov.nasa.jpl.aerie.contrib.streamline.debugging.Naming; import gov.nasa.jpl.aerie.contrib.streamline.modeling.Registrar; import gov.nasa.jpl.aerie.contrib.streamline.modeling.clocks.Clock; import gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.monads.DiscreteDynamicsMonad; @@ -12,9 +13,12 @@ import gov.nasa.jpl.aerie.contrib.streamline.unit_aware.Unit; import gov.nasa.jpl.aerie.contrib.streamline.unit_aware.UnitAware; import gov.nasa.jpl.aerie.contrib.streamline.unit_aware.UnitAwareResources; +import gov.nasa.jpl.aerie.merlin.framework.Result; import gov.nasa.jpl.aerie.merlin.framework.ValueMapper; import gov.nasa.jpl.aerie.merlin.protocol.model.EffectTrait; import gov.nasa.jpl.aerie.merlin.protocol.types.Duration; +import gov.nasa.jpl.aerie.merlin.protocol.types.SerializedValue; +import gov.nasa.jpl.aerie.merlin.protocol.types.ValueSchema; import java.time.Instant; import java.time.temporal.ChronoUnit; @@ -22,12 +26,12 @@ import java.util.function.BiPredicate; import java.util.function.Supplier; +import static gov.nasa.jpl.aerie.contrib.serialization.rulesets.BasicValueMappers.*; import static gov.nasa.jpl.aerie.contrib.streamline.core.CellRefV2.autoEffects; import static gov.nasa.jpl.aerie.contrib.streamline.core.CellRefV2.testing; import static gov.nasa.jpl.aerie.contrib.streamline.core.Expiring.expiring; import static gov.nasa.jpl.aerie.contrib.streamline.core.Expiry.expiry; -import static gov.nasa.jpl.aerie.contrib.streamline.core.MutableResource.resource; -import static gov.nasa.jpl.aerie.contrib.streamline.core.MutableResource.serializing; +import static gov.nasa.jpl.aerie.contrib.streamline.core.MutableResource.*; import static gov.nasa.jpl.aerie.contrib.streamline.core.Reactions.every; import static gov.nasa.jpl.aerie.contrib.streamline.core.Reactions.whenever; import static gov.nasa.jpl.aerie.contrib.streamline.core.Resources.*; @@ -35,7 +39,6 @@ import static gov.nasa.jpl.aerie.contrib.streamline.core.monads.ResourceMonad.pure; import static gov.nasa.jpl.aerie.contrib.streamline.debugging.Dependencies.addDependency; import static gov.nasa.jpl.aerie.contrib.streamline.debugging.Naming.*; -import static gov.nasa.jpl.aerie.contrib.streamline.modeling.clocks.ClockResources.clock; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.Discrete.discrete; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.DiscreteEffects.set; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.monads.DiscreteResourceMonad.*; @@ -50,11 +53,40 @@ public static Resource> constant(T value) { return result; } + // General discrete resource constructor public static DiscreteResourceBuilder discreteResource(T initialValue) { + return new DiscreteResourceBuilder().defaultValue(initialValue); + } + // Special cases for primitives, which (a) need a little special handling for double's effect trait, + // and (b) we can bake in the value mapper for them by default. + public static DiscreteResourceBuilder discreteResource(boolean initialValue) { + return new DiscreteResourceBuilder().defaultValue(initialValue).valueMapper($boolean()); + } + public static DiscreteResourceBuilder discreteResource(int initialValue) { + return new DiscreteResourceBuilder().defaultValue(initialValue).valueMapper($int()); + } + // Doubles are special, as they get a toleranced equality check by default for their effect trait. + public static DiscreteResourceBuilder discreteResource(double initialValue) { + return new DiscreteResourceBuilder() + .defaultValue(initialValue) + .valueMapper($double()) + .effectTrait(autoEffects(testing( + (CommutativityTestInput> input) -> DoubleUtils.areEqualResults( + input.original().extract(), + input.leftResult().extract(), + input.rightResult().extract())))); + } + public static DiscreteResourceBuilder discreteResource(String initialValue) { + return new DiscreteResourceBuilder().defaultValue(initialValue).valueMapper(string()); + } + // SAFETY - Enums are final, so initialValue.getClass() equals E, hence this assignment is guaranteed correct. + @SuppressWarnings("unchecked") + public static > DiscreteResourceBuilder discreteResource(E initialValue) { + return new DiscreteResourceBuilder().defaultValue(initialValue).valueMapper($enum(initialValue.getClass())); } - public class DiscreteResourceBuilder { + public static class DiscreteResourceBuilder { private ErrorCatching>> defaultValue; private String name; private ValueMapper valueMapper; @@ -66,32 +98,64 @@ public DiscreteResourceBuilder defaultValue(T defaultValue) { } public DiscreteResourceBuilder defaultValue(ErrorCatching>> defaultValue) { - assertNotSet("default value", this.defaultValue); this.defaultValue = defaultValue; return this; } public DiscreteResourceBuilder name(String name) { - assertNotSet("name", this.name); this.name = name; return this; } public DiscreteResourceBuilder valueMapper(ValueMapper valueMapper) { - assertNotSet("value mapper", this.valueMapper); this.valueMapper = valueMapper; return this; } + public DiscreteResourceBuilder notSaved() { + assertSet("default value", defaultValue); + return inconBehavior(notSaving(defaultValue)); + } + + public DiscreteResourceBuilder serialized() { + assertSet("name", name); + assertSet("default value", defaultValue); + assertSet("value mapper", valueMapper); + return inconBehavior(serializing(name, defaultValue, standardDynamicsMapper(standardDiscreteMapper(valueMapper)))); + } + + public static ValueMapper> standardDiscreteMapper(ValueMapper mapper) { + return new ValueMapper<>() { + @Override + public ValueSchema getValueSchema() { + return mapper.getValueSchema(); + } + + @Override + public Result, String> deserializeValue(SerializedValue serializedValue) { + return mapper.deserializeValue(serializedValue).mapSuccess(Discrete::discrete); + } + + @Override + public SerializedValue serializeValue(Discrete value) { + return mapper.serializeValue(value.extract()); + } + }; + } + public DiscreteResourceBuilder inconBehavior(InconBehavior>>> inconBehavior) { - assertNotSet("incon behavior", this.inconBehavior); this.inconBehavior = inconBehavior; return this; } - private void assertNotSet(String name, Object thing) { - if (thing != null) - throw new IllegalStateException(String.format("%s has already been set on this builder!", name)); + public DiscreteResourceBuilder effectTrait(EffectTrait>> effectTrait) { + this.effectTrait = effectTrait; + return this; + } + + private void assertSet(String name, Object thing) { + if (thing == null) + throw new IllegalStateException(String.format("%s has not been set on this builder!", name)); } // Terminal methods - these build and return the resource @@ -99,41 +163,24 @@ private void assertNotSet(String name, Object thing) { // TODO - would it be more convenient to make Registrar a singleton, like the incon manager? // Then it wouldn't have to be passed around just to give to these builders. // We already assume you have exactly one registrar, because building it initializes all the singletons... - public Resource> registered(Registrar registrar) { + public MutableResource> registered(Registrar registrar) { var result = notRegistered(); + assertSet("name", name); + assertSet("value mapper", valueMapper); registrar.discrete(name, result, valueMapper); return result; } - public Resource> notRegistered() { - return resource(inconBehavior, effectTrait); + public MutableResource> notRegistered() { + // If no incon behavior is set, default to serializing the value in the default way. + if (inconBehavior == null) serialized(); + assertSet("effect trait", effectTrait); + var result = resource(inconBehavior, effectTrait); + if (name != null) Naming.name(result, name); + return result; } } - // --- REWRITE START --- - - // General discrete cell resource constructor - public static MutableResource> discreteResource(T initialValue) { - return resource(discrete(initialValue)); - } - - // Annoyingly, we need to repeat the specialization for integer resources, so that - // discreteMutableResource(42) doesn't become a double resource, due to the next overload - public static MutableResource> discreteResource(int initialValue) { - return resource(discrete(initialValue)); - } - - // specialized constructor for doubles, because they require a toleranced equality comparison - public static MutableResource> discreteResource(double initialValue) { - return resource(discrete(initialValue), autoEffects(testing( - (CommutativityTestInput> input) -> DoubleUtils.areEqualResults( - input.original().extract(), - input.leftResult().extract(), - input.rightResult().extract())))); - } - - // --- REWRITE END --- - /** * Returns a condition that's satisfied whenever this resource is true. */ @@ -150,7 +197,9 @@ public static Condition when(Resource> resource) { * Cache resource, updating the cache when updatePredicate(cached value, resource value) is true. */ public static Resource> cache(Resource> resource, BiPredicate updatePredicate) { - final var cell = resource(resource.getDynamics()); + // Caches are logically derived from the resource they're caching, so should not be directly saved. + // Instead, the resource(s) feeding this cache should be saved, and used to reinitialize this. + final var cell = resource(notSaving(resource.getDynamics())); // TODO: Does the update predicate need to propagate expiry information? BiPredicate>>, ErrorCatching>>> liftedUpdatePredicate = (eCurrent, eNew) -> eCurrent.match( @@ -178,7 +227,7 @@ public static Resource> cache(Resource> resource, Bi * Sample valueSupplier once every samplePeriod. */ public static > Resource> sampled(Supplier valueSupplier, Resource samplePeriod) { - var result = discreteResource(valueSupplier.get()); + var result = discreteResource(valueSupplier.get()).notSaved().notRegistered(); every(() -> currentValue(samplePeriod, Duration.MAX_VALUE), () -> set(result, valueSupplier.get())); return result; @@ -191,8 +240,7 @@ public static > Resource> sampled */ public static Resource> precomputed( final V valueBeforeFirstEntry, final NavigableMap segments) { - var clock = clock(); - return signalling(bind(clock, (Clock clock$) -> { + return signalling(bind(simulationClock(), (Clock clock$) -> { var t = clock$.extract(); var entry = segments.floorEntry(t); var value = entry == null ? valueBeforeFirstEntry : entry.getValue(); diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java index 7545e510bb..35e6b7f80f 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java @@ -5,6 +5,7 @@ import gov.nasa.jpl.aerie.contrib.streamline.core.monads.DynamicsMonad; import gov.nasa.jpl.aerie.contrib.streamline.core.monads.ErrorCatchingMonad; import gov.nasa.jpl.aerie.contrib.streamline.debugging.Naming; +import gov.nasa.jpl.aerie.contrib.streamline.modeling.Registrar; import gov.nasa.jpl.aerie.contrib.streamline.modeling.black_box.*; import gov.nasa.jpl.aerie.contrib.streamline.modeling.clocks.Clock; import gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.Discrete; @@ -17,6 +18,8 @@ import gov.nasa.jpl.aerie.contrib.streamline.unit_aware.UnitAwareResources; import gov.nasa.jpl.aerie.contrib.streamline.utils.DoubleUtils; import gov.nasa.jpl.aerie.merlin.framework.Condition; +import gov.nasa.jpl.aerie.merlin.framework.ValueMapper; +import gov.nasa.jpl.aerie.merlin.protocol.model.EffectTrait; import gov.nasa.jpl.aerie.merlin.protocol.types.Duration; import org.apache.commons.lang3.mutable.MutableObject; @@ -30,10 +33,10 @@ import static gov.nasa.jpl.aerie.contrib.streamline.core.CellRefV2.autoEffects; import static gov.nasa.jpl.aerie.contrib.streamline.core.CellRefV2.testing; -import static gov.nasa.jpl.aerie.contrib.streamline.core.MutableResource.resource; import static gov.nasa.jpl.aerie.contrib.streamline.core.Expiring.expiring; import static gov.nasa.jpl.aerie.contrib.streamline.core.Expiring.neverExpiring; import static gov.nasa.jpl.aerie.contrib.streamline.core.Expiry.NEVER; +import static gov.nasa.jpl.aerie.contrib.streamline.core.MutableResource.*; import static gov.nasa.jpl.aerie.contrib.streamline.core.Reactions.wheneverDynamicsChange; import static gov.nasa.jpl.aerie.contrib.streamline.core.Resources.*; import static gov.nasa.jpl.aerie.contrib.streamline.core.monads.DynamicsMonad.bindEffect; @@ -47,7 +50,6 @@ import static gov.nasa.jpl.aerie.contrib.streamline.modeling.black_box.IntervalFunctions.byBoundingError; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.black_box.SecantApproximation.ErrorEstimates.errorByQuadraticApproximation; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.black_box.SecantApproximation.secantApproximation; -import static gov.nasa.jpl.aerie.contrib.streamline.modeling.clocks.ClockResources.clock; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.Discrete.discrete; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.DiscreteResources.assertThat; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.polynomial.Polynomial.polynomial; @@ -89,6 +91,75 @@ public static MutableResource polynomialResource(Polynomial initialD }))); } + public class PolynomialResourceBuilder { + private ErrorCatching> defaultValue; + private String name; + private InconBehavior>> inconBehavior; + private EffectTrait> effectTrait = autoEffects(testing( + (CommutativityTestInput input) -> { + Polynomial original = input.original(); + Polynomial left = input.leftResult(); + Polynomial right = input.rightResult(); + return left.degree() == right.degree() && + IntStream.rangeClosed(0, left.degree()).allMatch( + i -> DoubleUtils.areEqualResults( + original.getCoefficient(i), + left.getCoefficient(i), + right.getCoefficient(i))); + })); + + public PolynomialResourceBuilder defaultValue(Polynomial defaultValue) { + return defaultValue(DynamicsMonad.pure(defaultValue)); + } + + public PolynomialResourceBuilder defaultValue(ErrorCatching> defaultValue) { + this.defaultValue = defaultValue; + return this; + } + + public PolynomialResourceBuilder name(String name) { + this.name = name; + return this; + } + + public PolynomialResourceBuilder notSaved() { + assertSet("default value", defaultValue); + return inconBehavior(notSaving(defaultValue)); + } + + public PolynomialResourceBuilder serialized() { + assertSet("name", name); + assertSet("default value", defaultValue); + return inconBehavior(serializing(name, defaultValue, standardDynamicsMapper(null /* TODO - get auto value mapper for polynomial */))); + } + + public PolynomialResourceBuilder inconBehavior(InconBehavior>> inconBehavior) { + this.inconBehavior = inconBehavior; + return this; + } + + public PolynomialResourceBuilder effectTrait(EffectTrait> effectTrait) { + this.effectTrait = effectTrait; + return this; + } + + private void assertSet(String name, Object thing) { + if (thing == null) + throw new IllegalStateException(String.format("%s has not been set on this builder!", name)); + } + + // Terminal methods - these build and return the resource + + public MutableResource notRegistered() { + // If no incon behavior is set, default to serializing the value in the default way. + if (inconBehavior == null) serialized(); + assertSet("effect trait", effectTrait); + var result = resource(inconBehavior, effectTrait); + if (name != null) Naming.name(result, name); + return result; + } + } + /** * Treat a discrete resource as a polynomial with constant profile segments. */ @@ -207,8 +278,7 @@ public static Resource precomputed(final NavigableMap { + return signalling(bind(simulationClock(), (Clock clock$) -> { var t = clock$.extract(); var start = segments.floorEntry(t); var end = segments.higherEntry(t);