Linear transient analysis model improvements

[samreid]

During discussion in #758 I decided to recommend significant model improvements. This can be through a rewrite, but may be possible through progressive changes. This part of the model is around 2000 lines of code. It is complicated but not that much code. Here are some problems that should be addressed:

• The code is split up into 21 different (mostly small) files, which don't do much
• Get rid of the *Adapter subclasses, and use a Map or something else at that level
• Fix ModifiedNodalAnalysisCircuitElement is too general #762
• Capacitor and Inductor should not be ModifiedCircuitAnalysisElements
• DynamicCapacitorAdapter is constructed with a Capacitor, and creates /composesnew DynamicCircuitCapacitor, and extends DynamicCapacitor. This is all too many levels of adapters and confusion.
• We may be able to keep the batteries/resistors/currents as they are, but just improve the layer for capacitors and inductors
• @ariel-phet recommended removing current sources until they are needed for something.
• A better name for the entry point, perhaps LinearTransientAnalysis

I've been putting this off for a long time hoping things were "good enough" for publication, but we keep finding odd bugs like #758. Now that we have TypeScript, I feel like we have a safety net and a way to make bigger changes like this more safely.

[samreid]

Path that eliminates ModifiedNodalAnalysisCircuitElement as a component of DynamicCapacitor, but lookups or currentSolution stamps are failing:

Index: js/model/analysis/DynamicCircuitSolution.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/analysis/DynamicCircuitSolution.ts b/js/model/analysis/DynamicCircuitSolution.ts
--- a/js/model/analysis/DynamicCircuitSolution.ts	(revision 4982a4d5221d2300c1e5cc6de6a918e864d29b9d)
+++ b/js/model/analysis/DynamicCircuitSolution.ts	(date 1635087690382)
@@ -39,33 +39,28 @@
    */
   getCurrent( element: ModifiedNodalAnalysisCircuitElement | DynamicCapacitor | DynamicInductor ) {
 
-    // Scaffolding tests for TypeScript migration
+
     if ( element instanceof ModifiedNodalAnalysisCircuitElement ) {
-      assert && assert( element.hasOwnProperty( 'currentSolution' ) );
-    }
-    if ( element instanceof DynamicCapacitor || element instanceof DynamicInductor ) {
-      assert && assert( !element.hasOwnProperty( 'currentSolution' ) );
-    }
 
-    // For resistors with r>0, Ohm's Law gives the current.  For components with no resistance (like closed switch or
-    // 0-resistance battery), the current is given by the matrix solution.
-    if ( element instanceof ModifiedNodalAnalysisCircuitElement && element.currentSolution !== null ) {
-      return element.currentSolution;
-    }
+
+      // For components with no resistance (like closed switch or 0-resistance battery), the current is given by the
+      // matrix solution.
+      if ( element.currentSolution !== null ) {
+        return element.currentSolution;
+      }
+      else {
 
-    // Support
-    const companion = _.find( this.currentCompanions, c => c.element === element ||
-                                                           c.element.dynamicCircuitCapacitor === element ||
-                                                           c.element.dynamicCircuitInductor === element );
-
-    if ( companion ) {
+        // For resistors with r>0, Ohm's Law gives the current.
+        return this.mnaSolution.getCurrentForResistor( element );
+      }
+    }
+    else {
+
+      const companion = _.find( this.currentCompanions, c => c.element === element ||
+                                                             c.element.id === element.id ||
+                                                             c.element.dynamicCircuitInductor === element );
       return companion.getValueForSolution( this.mnaSolution );
     }
-    else {
-
-      assert && assert( element instanceof ModifiedNodalAnalysisCircuitElement );
-      return this.mnaSolution.getCurrentForResistor( element as ModifiedNodalAnalysisCircuitElement );
-    }
   }
 
   /**
Index: js/model/analysis/DynamicInductor.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/analysis/DynamicInductor.ts b/js/model/analysis/DynamicInductor.ts
--- a/js/model/analysis/DynamicInductor.ts	(revision 4982a4d5221d2300c1e5cc6de6a918e864d29b9d)
+++ b/js/model/analysis/DynamicInductor.ts	(date 1635087127879)
@@ -6,8 +6,9 @@
   readonly dynamicCircuitInductor: ModifiedNodalAnalysisCircuitElement;
   readonly state: DynamicElementState;
   readonly inductance: number;
+  readonly id: number;
 
-  constructor( dynamicCircuitInductor: ModifiedNodalAnalysisCircuitElement, state: DynamicElementState, inductance: number ) {
+  constructor( dynamicCircuitInductor: ModifiedNodalAnalysisCircuitElement, state: DynamicElementState, inductance: number, id: number ) {
 
     // @public {Inductor}
     this.dynamicCircuitInductor = dynamicCircuitInductor;
@@ -16,6 +17,8 @@
     this.state = state;
 
     this.inductance = inductance;
+
+    this.id = id;
   }
 }
 
Index: js/model/analysis/DynamicCapacitor.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/analysis/DynamicCapacitor.ts b/js/model/analysis/DynamicCapacitor.ts
--- a/js/model/analysis/DynamicCapacitor.ts	(revision 4982a4d5221d2300c1e5cc6de6a918e864d29b9d)
+++ b/js/model/analysis/DynamicCapacitor.ts	(date 1635047212585)
@@ -1,18 +1,16 @@
 // Copyright 2021, University of Colorado Boulder
 import DynamicElementState from './DynamicElementState.js';
-import ModifiedNodalAnalysisCircuitElement from './mna/ModifiedNodalAnalysisCircuitElement.js';
 
 class DynamicCapacitor {
-  readonly dynamicCircuitCapacitor: ModifiedNodalAnalysisCircuitElement;
   readonly state: DynamicElementState;
   capacitorVoltageNode0: string | null;
   capacitorVoltageNode1: string | null;
-  capacitance: number;
+  readonly capacitance: number;
+  readonly node0: string;
+  readonly node1: string;
+  readonly id: number;
 
-  constructor( dynamicCircuitCapacitor: ModifiedNodalAnalysisCircuitElement, state: DynamicElementState, capacitance: number ) {
-
-    // @public {DynamicCircuit.Capacitor}
-    this.dynamicCircuitCapacitor = dynamicCircuitCapacitor;
+  constructor( node0: string, node1: string, state: DynamicElementState, capacitance: number, id: number ) {
 
     // @public {DynamicElementState}
     this.state = state;
@@ -21,6 +19,10 @@
     this.capacitorVoltageNode0 = null;
     this.capacitorVoltageNode1 = null;
     this.capacitance = capacitance;
+    this.node0 = node0;
+    this.node1 = node1;
+
+    this.id = id;
   }
 }
 
Index: js/model/analysis/mna/ModifiedNodalAnalysisSolution.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/analysis/mna/ModifiedNodalAnalysisSolution.ts b/js/model/analysis/mna/ModifiedNodalAnalysisSolution.ts
--- a/js/model/analysis/mna/ModifiedNodalAnalysisSolution.ts	(revision 4982a4d5221d2300c1e5cc6de6a918e864d29b9d)
+++ b/js/model/analysis/mna/ModifiedNodalAnalysisSolution.ts	(date 1635087364082)
@@ -103,8 +103,10 @@
    * @returns {number}
    * @public
    */
-  getCurrentForResistor( resistor: ModifiedNodalAnalysisCircuitElement ) {
-    assert && assert( resistor.mnaValue > 0, 'resistor must have resistance to use Ohms Law' );
+  getCurrentForResistor( resistor: any ) {
+    // assert && assert( resistor.mnaValue > 0, 'resistor must have resistance to use Ohms Law' );
+
+    const resistance = resistor.resistance || resistor.mnaValue; // TODO
 
     // To help understand the minus sign here:
     // Imagine a resistor that goes from node r0 to r1, with a conventional current flowing from r0 to r1.  Then
@@ -113,7 +115,7 @@
     // Conversely, if v0>v1, then voltage is negative, so for the conventional current to flow to the right we must
     // multiply it by a negative.
     // Same sign as Java, see https://github.com/phetsims/circuit-construction-kit-common/issues/758
-    return -this.getVoltage( resistor ) / resistor.mnaValue;
+    return -this.getVoltage( resistor ) / resistance;
   }
 
   /**
Index: js/model/analysis/LinearTransientAnalysis.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/analysis/LinearTransientAnalysis.ts b/js/model/analysis/LinearTransientAnalysis.ts
--- a/js/model/analysis/LinearTransientAnalysis.ts	(revision 4982a4d5221d2300c1e5cc6de6a918e864d29b9d)
+++ b/js/model/analysis/LinearTransientAnalysis.ts	(date 1635087180023)
@@ -35,6 +35,8 @@
 // constants
 const TIMESTEP_SUBDIVISIONS = new TimestepSubdivisions<DynamicState>();
 
+let id = 0;
+
 class LinearTransientAnalysis {
 
   /**
@@ -94,25 +96,26 @@
           // no element for an open switch
         }
         else if ( circuitElement instanceof Capacitor ) {
-          const dynamicCircuitCapacitor = new ModifiedNodalAnalysisCircuitElement(
+          const dynamicCapacitor = new DynamicCapacitor(
             circuitElement.startVertexProperty.value.index + '',
             circuitElement.endVertexProperty.value.index + '',
-            null,
-            0
+            new DynamicElementState( circuitElement.mnaVoltageDrop, circuitElement.mnaCurrent ),
+            circuitElement.capacitanceProperty.value,
+            id++
           );
-
-          const dynamicCapacitor = new DynamicCapacitor( dynamicCircuitCapacitor, new DynamicElementState( circuitElement.mnaVoltageDrop, circuitElement.mnaCurrent ), circuitElement.capacitanceProperty.value );
           dynamicCapacitors.push( dynamicCapacitor );
           backwardMap.set( dynamicCapacitor, circuitElement );
         }
         else if ( circuitElement instanceof Inductor ) {
 
           const dynamicInductor = new DynamicInductor( new ModifiedNodalAnalysisCircuitElement(
-            circuitElement.startVertexProperty.value.index + '',
-            circuitElement.endVertexProperty.value.index + '',
-            null,
-            0
-          ), new DynamicElementState( circuitElement.mnaVoltageDrop, circuitElement.mnaCurrent ), circuitElement.inductanceProperty.value );
+              circuitElement.startVertexProperty.value.index + '',
+              circuitElement.endVertexProperty.value.index + '',
+              null,
+              0
+            ), new DynamicElementState( circuitElement.mnaVoltageDrop, circuitElement.mnaCurrent ),
+            circuitElement.inductanceProperty.value,
+            id++ );
           backwardMap.set( dynamicInductor, circuitElement );
           dynamicInductors.push( dynamicInductor );
         }
@@ -189,8 +192,8 @@
     } );
     dynamicCapacitors.forEach( dynamicCapacitor => {
       const capacitor = backwardMap.get( dynamicCapacitor ) as Capacitor;
-      capacitor.currentProperty.value = circuitResult.getTimeAverageCurrent( dynamicCapacitor.dynamicCircuitCapacitor );
-      capacitor.mnaCurrent = CCKCUtils.clampMagnitude( circuitResult.getInstantaneousCurrent( dynamicCapacitor.dynamicCircuitCapacitor ) );
+      capacitor.currentProperty.value = circuitResult.getTimeAverageCurrent( dynamicCapacitor );
+      capacitor.mnaCurrent = CCKCUtils.clampMagnitude( circuitResult.getInstantaneousCurrent( dynamicCapacitor ) );
 
       assert && assert( typeof dynamicCapacitor.capacitorVoltageNode1 === 'string' );
       assert && assert( typeof dynamicCapacitor.capacitorVoltageNode0 === 'string' );
Index: js/model/analysis/DynamicCircuit.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/analysis/DynamicCircuit.ts b/js/model/analysis/DynamicCircuit.ts
--- a/js/model/analysis/DynamicCircuit.ts	(revision 4982a4d5221d2300c1e5cc6de6a918e864d29b9d)
+++ b/js/model/analysis/DynamicCircuit.ts	(date 1635087174261)
@@ -104,15 +104,15 @@
       // |V|=|IReq| and sign is unchanged since the conventional current flows from high to low voltage.
       const companionVoltage = dynamicCapacitor.state.voltage - companionResistance * dynamicCapacitor.state.current;
 
-      const battery = new ModifiedNodalAnalysisCircuitElement( dynamicCapacitor.dynamicCircuitCapacitor.nodeId0 + '', newNode1, null, companionVoltage );
+      const battery = new ModifiedNodalAnalysisCircuitElement( dynamicCapacitor.node0, newNode1, null, companionVoltage );
       const resistor = new ModifiedNodalAnalysisCircuitElement( newNode1, newNode2, null, companionResistance );
-      const resistor2 = new ModifiedNodalAnalysisCircuitElement( newNode2, dynamicCapacitor.dynamicCircuitCapacitor.nodeId1 + '', null, resistanceTerm );
+      const resistor2 = new ModifiedNodalAnalysisCircuitElement( newNode2, dynamicCapacitor.node1 + '', null, resistanceTerm );
 
       companionBatteries.push( battery );
       companionResistors.push( resistor );
       companionResistors.push( resistor2 );
 
-      dynamicCapacitor.capacitorVoltageNode0 = dynamicCapacitor.dynamicCircuitCapacitor.nodeId0;
+      dynamicCapacitor.capacitorVoltageNode0 = dynamicCapacitor.node0;
       dynamicCapacitor.capacitorVoltageNode1 = newNode2;
 
       // We need to be able to get the current for this component. In series, so the current is the same through both.
@@ -231,7 +231,7 @@
         solution.getNodeVoltage( dynamicCapacitor.capacitorVoltageNode1! ) - solution.getNodeVoltage( dynamicCapacitor.capacitorVoltageNode0! ),
         solution.getCurrent( dynamicCapacitor )
       );
-      return new DynamicCapacitor( dynamicCapacitor.dynamicCircuitCapacitor, newState, dynamicCapacitor.capacitance );
+      return new DynamicCapacitor( dynamicCapacitor.node0, dynamicCapacitor.node1, newState, dynamicCapacitor.capacitance, dynamicCapacitor.id );
     } );
     const updatedInductors = this.dynamicInductors.map( dynamicInductor => {
       const newState = new DynamicElementState(
@@ -239,7 +239,7 @@
         solution.getNodeVoltage( dynamicInductor.dynamicCircuitInductor.nodeId1 ) - solution.getNodeVoltage( dynamicInductor.dynamicCircuitInductor.nodeId0 ),
         solution.getCurrent( dynamicInductor )
       );
-      return new DynamicInductor( dynamicInductor.dynamicCircuitInductor, newState, dynamicInductor.inductance );
+      return new DynamicInductor( dynamicInductor.dynamicCircuitInductor, newState, dynamicInductor.inductance, dynamicInductor.id );
     } );
 
     return new DynamicCircuit( this.resistorAdapters, this.resistiveBatteryAdapters, updatedCapacitors, updatedInductors );
Index: js/model/analysis/CircuitResult.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/analysis/CircuitResult.ts b/js/model/analysis/CircuitResult.ts
--- a/js/model/analysis/CircuitResult.ts	(revision 4982a4d5221d2300c1e5cc6de6a918e864d29b9d)
+++ b/js/model/analysis/CircuitResult.ts	(date 1635087472873)
@@ -32,7 +32,11 @@
   getTimeAverageCurrent( element: ModifiedNodalAnalysisCircuitElement | DynamicCapacitor | DynamicInductor ) {
     let weightedSum = 0.0;
     this.resultSet.states.forEach( ( stateObject: any ) => {
-      weightedSum += stateObject.state.dynamicCircuitSolution.getCurrent( element ) * stateObject.dt;
+      const currentTerm = stateObject.state.dynamicCircuitSolution.getCurrent( element );
+
+      assert && assert( !isNaN( currentTerm ) );
+      weightedSum += currentTerm * stateObject.dt;
+      assert && assert( !isNaN( weightedSum ) );
     } );
     const number = weightedSum / this.resultSet.getTotalTime();
     assert && assert( !isNaN( number ) );
@@ -45,7 +49,7 @@
    * @returns {number}
    * @public
    */
-  getInstantaneousCurrent( element: ModifiedNodalAnalysisCircuitElement ) {
+  getInstantaneousCurrent( element: ModifiedNodalAnalysisCircuitElement | DynamicCapacitor | DynamicInductor ) {
     return this.getFinalState().dynamicCircuitSolution!.getCurrent( element );
   }
 
Index: js/model/analysis/DynamicCircuitTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/analysis/DynamicCircuitTests.ts b/js/model/analysis/DynamicCircuitTests.ts
--- a/js/model/analysis/DynamicCircuitTests.ts	(revision 4982a4d5221d2300c1e5cc6de6a918e864d29b9d)
+++ b/js/model/analysis/DynamicCircuitTests.ts	(date 1635087174263)
@@ -23,10 +23,10 @@
 const testVRCCircuit = ( v: number, r: number, c: number, assert: Assert ) => {
   const resistor = new ModifiedNodalAnalysisCircuitElement( '1', '2', null, r );
   const battery = new DynamicCircuitResistiveBattery( '0', '1', v, 0 );
-  const capacitor = new DynamicCapacitor(
-    new ModifiedNodalAnalysisCircuitElement( '2', '0', null, 0 ),
+  const capacitor = new DynamicCapacitor( '2', '0',
     new DynamicElementState( 0.0, v / r ),
-    c
+    c,
+    0
   );
 
   let dynamicCircuit = new DynamicCircuit( [ resistor ], [ battery ], [ capacitor ], [] );
@@ -67,7 +67,7 @@
 const testVRLCircuit = ( V: number, R: number, L: number, assert: Assert ) => {
   const resistor = new ModifiedNodalAnalysisCircuitElement( '1', '2', null, R );
   const battery = new DynamicCircuitResistiveBattery( '0', '1', V, 0 );
-  const inductor = new DynamicInductor( new ModifiedNodalAnalysisCircuitElement( '2', '0', null, 0 ), new DynamicElementState( V, 0.0 ), L );
+  const inductor = new DynamicInductor( new ModifiedNodalAnalysisCircuitElement( '2', '0', null, 0 ), new DynamicElementState( V, 0.0 ), L, 0 );
   let circuit = new DynamicCircuit( [ resistor ], [ battery ], [], [ inductor ] );
 
   // let x = '';

[samreid]

This code is in much better shape. This was inspired by #758 and it was possible to make these changes efficiently and safely thanks to TypeScript and #749. We will test for regressions in #758, closing.