Matt updates

contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/CellRefV2.java

@@ -14,19 +14,8 @@
 import static gov.nasa.jpl.aerie.contrib.streamline.debugging.Naming.*;
 import static gov.nasa.jpl.aerie.merlin.protocol.types.Duration.ZERO;
 
-// MD: The V2 naming is unfortunate - I can see the argument for moving this into the framework
-// MD: On the other hand, this is never called from user code...
-// MD: Maybe a javadoc?
-
-// MD: How general is this class? Does it completely obviate the need for CellRef?
-
 /**
  * Utility class for a simplified allocate method.
- *
- * For starters, it blurs the distinction between event
- * and effect, kind of like the allocate/2 method in CellRef.
- *
- *
  */
 public final class CellRefV2 {
   private CellRefV2() {}
@@ -90,8 +79,6 @@ public static <D extends Dynamics<?, D>> EffectTrait<DynamicsEffect<D>> autoEffe
       if (commutativityTest.test(new CommutativityTestInput<>(x, lrx, rlx))) {
         return lrx;
       } else {
-        // MD: Future work: is there a way we can report this cleanly to the user? Does the
-        // error catching machinery already do that?
         throw new UnsupportedOperationException(
             "Detected non-commuting concurrent effects!");
       }
@@ -152,8 +139,6 @@ public DynamicsEffect<D> concurrently(final DynamicsEffect<D> left, final Dynami
           name(result, "(%s) and (%s)".formatted(getEffectName(left), getEffectName(right)));
           return result;
         } catch (Throwable e) {
-          // MD: Very broad catch... I wonder if this can cause any problems with replaying tasks, which rely on
-          // exceptions for control flow
           final DynamicsEffect<D> result = $ -> failure(e);
           name(result, "Failed to combine concurrent effects: (%s) and (%s)".formatted(
                   getEffectName(left), getEffectName(right)));

contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/MutableResource.java

@@ -41,7 +41,6 @@ static <D extends Dynamics<?, D>> MutableResource<D> resource(ErrorCatching<Expi
 
   static <D extends Dynamics<?, D>> MutableResource<D> resource(ErrorCatching<Expiring<D>> initial, EffectTrait<DynamicsEffect<D>> effectTrait) {
     MutableResource<D> result = new MutableResource<>() {
-      // MD: It was at this moment that Matt realized that MutableResource is an interface. But why...?
       private final CellRef<DynamicsEffect<D>, Cell<D>> cell = allocate(initial, effectTrait);
 
       @Override
@@ -68,8 +67,6 @@ private void augmentEffectName(DynamicsEffect<D> effect) {
     return result;
   }
 
-  // MD: Musing... I wonder if we want to encourage MutableResource to be used by end-users, or by "library" authors.
-  // i.e. does this slot in between `Register` and `Cell`, or does it replace `Register`?
   static <D extends Dynamics<?, D>> void set(MutableResource<D> resource, D newDynamics) {
     resource.emit("Set " + newDynamics, DynamicsMonad.effect(x -> newDynamics));
   }
@@ -106,31 +103,3 @@ static void detectBusyCells() {
 final class MutableResourceFlags {
   public static boolean DETECT_BUSY_CELLS = false;
 }
-
-/*
-Add MutableResource
-
-Adds MutableResource and supporting types for defining cells and effects.
-
-This design emphasizes separation of concerns in two primary ways:
-* First, since every resource dynamics carries an expiry and stepping behavior,
-  we don't need to define a different cell type depending on how that value is computed (like an Accumulator)
-  nor by what kind of dynamics are stored (e.g. Discrete vs. Real).
-* Second, since the DynamicsEffect interface defines a fully general effect type,
-  we don't need to define a different cell type depending on the supported class of effects.
-  Taken together, we can define a single cell type.
-
-This design also seeks to reduce overhead for modelers to handle effects the "right" way.
-By this, we mean using semantically correct effects, rather than (ab)using Registers for everything.
-* Instead of defining a new type for effects, we use a general DynamicsEffect interface.
-  We also have the DynamicsMonad.effect method, so effects can be written against the base dynamics type,
-  often as a small in-line lambda.
-* To support these "black-box" effects, we use an "automatic" effect trait by default,
-  which tests concurrent effects for commutativity.
-  Since effects are rarely concurrent in practice, this is performant enough in most use cases.
-  Furthermore, it combines with the error-handling wrapper to bubble-up useful error messages,
-  as well as let independent portions of the simulation continue normally.
-
-Taken together, the above means there's a single "default" way to build a cell,
-which provides enough flexibility and performance for most use cases.
- */

contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/monads/ErrorCatchingMonad.java

@@ -24,7 +24,6 @@ public static <A, B> ErrorCatching<B> apply(ErrorCatching<A> a, ErrorCatching<Fu
       try {
         return success(f$.apply(a$));
       } catch (Throwable e) {
-        // MD: Broad catch
         return failure(e);
       }
     }, ErrorCatching::failure), ErrorCatching::failure);

contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/Registrar.java

@@ -183,7 +183,6 @@ private static <D> gov.nasa.jpl.aerie.merlin.framework.Resource<D> wrapErrors(St
       try {
         return resource.getDynamics();
       } catch (Throwable e) {
-        // MD: Broad catch... hmm...
         throw new RuntimeException("Error affecting " + resourceName, e);
       }
     };

contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/Polynomial.java

@@ -25,10 +25,19 @@
 import static gov.nasa.jpl.aerie.merlin.protocol.types.Duration.ZERO;
 import static org.apache.commons.math3.analysis.polynomials.PolynomialsUtils.shift;
 
+/**
+ * An implementation of Polynomial Dynamics
+ * @param coefficients an array of polynomial coefficients, where an entry's index in the array corresponds to the degree of that coefficient
+ */
 public record Polynomial(double[] coefficients) implements Dynamics<Double, Polynomial> {
 
   // TODO: Add Duration parameter for unit of formal parameter?
-  // MD: Is the convention to provide coefficients from least significant to most significant? Can we document that?
+  // MD: What does the above TODO comment mean?
+  /**
+   *
+   * @param coefficients the polynomial coefficients, from least to most significant
+   * @return a Polynomial with the given coefficients
+   */
   public static Polynomial polynomial(double... coefficients) {
     int n = coefficients.length;
     if (n == 0) {

contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/unit_aware/Unit.java

@@ -26,7 +26,6 @@ private Unit(final Dimension dimension, final double multiplier, final String lo
 
   public static Unit createBase(final String shortName, final String longName, final Dimension dimension) {
     // TODO: Track base units, to detect and prevent collisions
-    // MD: Should this be in a separate PR?
     assert(dimension.isBase());
     return new Unit(dimension, 1, longName, shortName);
   }

merlin-framework/src/main/java/gov/nasa/jpl/aerie/merlin/framework/CellRef.java

@@ -37,9 +37,8 @@ CellRef<Event, State> allocate(
   }
 
   public static <Effect, State>
-  CellRef<Effect, State> allocate(final State initialState, final CellType<Effect, State> applicator) {
-    // MD: Lingering terminology: consider dropping the word `applicator`
-    return allocate(initialState, applicator, $ -> $);
+  CellRef<Effect, State> allocate(final State initialState, final CellType<Effect, State> cellType) {
+    return allocate(initialState, cellType, $ -> $);
   }
 
   public State get() {