currentProperty, powerDissipatedProperty and powerGeneratedProperty updating slightly every frame in connected circuit

[matthew-blackman]

Connecting a circuit with the data stream running shows rapid logs like the following:

circuitConstructionKitDc.introScreen.model.circuit.batteryGroup.battery_0.powerGeneratedProperty changed {"oldValue":8.09991906553702,"newValue":8.099920065564913}

circuitConstructionKitDc.introScreen.model.circuit.lightBulbGroup.lightBulb_0.currentProperty changed {"oldValue":0.8999910072818911,"newValue":0.8999909974817832}

circuitConstructionKitDc.introScreen.model.circuit.lightBulbGroup.lightBulb_0.powerDissipatedProperty changed {"oldValue":8.099838131882729,"newValue":8.099837955482553}

It appears the current and power properties are being set to slightly new values every frame.

[samreid]

In further testing, I saw this error occurred some times and not other times, even depending on the wire length. So it seems to be a roundoff error for certain configurations. I tried rounding off the current values in the model like so and that seemed to prevent the problem.

```diff Subject: [PATCH] Dispose stringProperty, see https://github.com/phetsims/circuit-construction-kit-common/issues/840 --- Index: js/model/CircuitElement.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/model/CircuitElement.ts b/js/model/CircuitElement.ts --- a/js/model/CircuitElement.ts (revision 2697d0490a570a467854246a9956b00ceea77150) +++ b/js/model/CircuitElement.ts (date 1669076924250) @@ -177,6 +177,7 @@ } ); this.currentProperty.link( current => { assert && assert( !isNaN( current ) ); + console.log( 'current, ' + current ); } ); // we assign the directionality based on the initial current direction, so the initial current is always positive. Index: js/model/analysis/LTAStateSet.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/model/analysis/LTAStateSet.ts b/js/model/analysis/LTAStateSet.ts --- a/js/model/analysis/LTAStateSet.ts (revision 2697d0490a570a467854246a9956b00ceea77150) +++ b/js/model/analysis/LTAStateSet.ts (date 1669077061543) @@ -4,6 +4,7 @@ import LTAState from './LTAState.js'; import CoreModel from './CoreModel.js'; import MNAResistor from './mna/MNAResistor.js'; +import Utils from '../../../../dot/js/Utils.js'; type Element = { dt: number; @@ -36,7 +37,7 @@ } ); const number = weightedSum / totalTime; assert && assert( !isNaN( number ) ); - return number; + return Utils.roundToInterval( number, 1E-6 ); } public getTimeAverageCurrentForCoreModel( element: CoreModel ): number { @@ -48,7 +49,8 @@ } ); const number = weightedSum / totalTime; assert && assert( !isNaN( number ) ); - return number; + + return Utils.roundToInterval( number, 1E-6 ); } /** ```

I'm not sure if rounding off the model could cause other problems though. Let's discuss.

[samreid]

@matthew-blackman and I want to track the inputs to the analysis and see if we are getting different outputs (even if at 1E-9) for the same inputs.

[matthew-blackman]

Patch from 12/14/22 with @samreid :

```diff Subject: [PATCH] Patch from work with SR 12/14/22 --- Index: js/model/LightBulb.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/model/LightBulb.ts b/js/model/LightBulb.ts --- a/js/model/LightBulb.ts (revision 13048a728d780aad982b483389ccd17344d665ed) +++ b/js/model/LightBulb.ts (date 1671048207953) @@ -135,6 +135,11 @@ // Fill in the chargePathLength this.chargePathLength = this.getPathLength(); + + this.currentProperty.link( current => { + assert && assert( !isNaN( current ) ); + console.log( 'current, ' + current ); + } ); } // Updates the charge path length when the view changes between lifelike/schematic 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 13048a728d780aad982b483389ccd17344d665ed) +++ b/js/model/analysis/LinearTransientAnalysis.ts (date 1671049945840) @@ -170,7 +170,8 @@ } ); ltaResistors.forEach( resistorAdapter => { const circuitElement = resistorMap.get( resistorAdapter )!; - circuitElement.currentProperty.value = circuitResult.getTimeAverageCurrent( resistorAdapter ); + // circuitElement.currentProperty.value = circuitResult.getTimeAverageCurrent( resistorAdapter ); + circuitElement.tempCurrent = circuitResult.getTimeAverageCurrent( resistorAdapter ); } ); ltaCapacitors.forEach( ltaCapacitor => { const capacitor = capacitorMap.get( ltaCapacitor )!; @@ -193,7 +194,8 @@ // zero out currents on open branches nonParticipants.forEach( circuitElement => { - circuitElement.currentProperty.value = 0; + // circuitElement.currentProperty.value = 0; + circuitElement.tempCurrent = 0; // Clear disconnected real light bulbs if ( circuitElement instanceof LightBulb && circuitElement.real ) { @@ -288,6 +290,10 @@ unsolvedVertices.forEach( v => { assert && assert( visited.includes( v ), 'unsolved vertex ' + v.tandem.phetioID + ' should be visited.' ); } ); + + circuit.circuitElements.forEach( circuitElement => { + circuitElement.currentProperty.value = circuitElement.tempCurrent; + } ); } } Index: js/model/analysis/LTACircuit.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/model/analysis/LTACircuit.ts b/js/model/analysis/LTACircuit.ts --- a/js/model/analysis/LTACircuit.ts (revision 13048a728d780aad982b483389ccd17344d665ed) +++ b/js/model/analysis/LTACircuit.ts (date 1671048195410) @@ -33,9 +33,9 @@ public readonly ltaInductors: LTAInductor[]; public constructor( ltaResistors: MNAResistor[], - ltaBatteries: LTAResistiveBattery[], - ltaCapacitors: LTACapacitor[], - ltaInductors: LTAInductor[] ) { + ltaBatteries: LTAResistiveBattery[], + ltaCapacitors: LTACapacitor[], + ltaInductors: LTAInductor[] ) { this.ltaResistors = ltaResistors; this.ltaBatteries = ltaBatteries; @@ -47,7 +47,6 @@ * Solving the companion model is the same as propagating forward in time by dt. */ public solvePropagate( dt: number ): LTASolution { - const companionBatteries: MNABattery[] = []; const companionResistors: MNAResistor[] = []; const companionCurrents: MNACurrent[] = []; Index: js/model/Circuit.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/model/Circuit.ts b/js/model/Circuit.ts --- a/js/model/Circuit.ts (revision 13048a728d780aad982b483389ccd17344d665ed) +++ b/js/model/Circuit.ts (date 1671050074525) @@ -1448,7 +1448,8 @@ let sum = 0; neighbors.forEach( neighbor => { const sign = neighbor.startVertexProperty.value === vertex ? +1 : -1; - const current = sign * neighbor.currentProperty.value; + // const current = sign * neighbor.currentProperty.value; + const current = sign * neighbor.tempCurrent; sum += current; } ); @@ -1460,7 +1461,8 @@ const overflow = sum / unlockedNeighbors.length; unlockedNeighbors.forEach( neighbor => { const sign = neighbor.startVertexProperty.value === vertex ? +1 : -1; - neighbor.currentProperty.value += -sign * overflow; + // neighbor.currentProperty.value += -sign * overflow; + neighbor.tempCurrent += -sign * overflow; locked.push( neighbor ); } ); } Index: js/model/CircuitElement.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/model/CircuitElement.ts b/js/model/CircuitElement.ts --- a/js/model/CircuitElement.ts (revision 13048a728d780aad982b483389ccd17344d665ed) +++ b/js/model/CircuitElement.ts (date 1671047822665) @@ -133,6 +133,7 @@ public readonly isDisposableProperty: BooleanProperty; public isValueDisplayableProperty: BooleanProperty; public labelStringProperty: StringProperty; + public tempCurrent: number; public constructor( startVertex: Vertex, endVertex: Vertex, chargePathLength: number, tandem: Tandem, providedOptions?: CircuitElementOptions ) { assert && assert( startVertex !== endVertex, 'startVertex cannot be the same as endVertex' ); @@ -185,6 +186,8 @@ assert && assert( !isNaN( current ) ); } ); + this.tempCurrent = 0; + // we assign the directionality based on the initial current direction, so the initial current is always positive. // see https://github.com/phetsims/circuit-construction-kit-common/issues/508 this.currentSenseProperty = new EnumerationProperty( CurrentSense.UNSPECIFIED, { ```

[matthew-blackman]

@arouinfar we have found the source of this issue, which is that the currentProperty is being set twice per frame, once before conserveCurrent is called and then again afterwards. @samreid and I found that this could be solved by adding a tempCurrent value to components and setting tempCurrent before conserveCurrent, and finally using tempCurrent to set currentProperty after conserveCurrent has run.

@samreid and I are 90% confident that this solution would take about 1-4 hours to implement. This issue could also be addressed via communication/documentation with users, informing them where the current values are coming from. Thoughts on how to proceed @arouinfar?

Note that this would slightly increase the complexity of the code, rather than simplify the model logic. It would look simpler from the user perspective, however.

[arouinfar]

@samreid @matthew-blackman thanks for investigating. I think we can defer this for now. The data stream is underutilized and we have a deferred epic to design it, https://github.com/phetsims/phet-io/issues/1734. In the meantime, we can designate these properties phetioHighFrequency: true.

This issue could also be addressed via communication/documentation with users, informing them where the current values are coming from.

Documentation sounds great. Any ideas where we should document this? Is there sim-specific documentation for developers? Currently, examples.md focuses on the phetioIDs relevant to specific customization, but maybe we can add a notes section to the bottom.

[samreid]

I feel it should be a section in examples.md since that is the "sim specific" documentation. But I'm not sure how it is an "example". Maybe there would be a section like:

Current and Power Readings

Due to an implementation detail in the simulation, the current and power are computed in a 2-phase algorithm, and have their values set twice during one step. So if you are observing values like circuitConstructionKitDc.introScreen.model.circuit.lightBulbGroup.lightBulb_0.currentProperty or circuitConstructionKitDc.introScreen.model.circuit.batteryGroup.battery_0.powerGeneratedProperty in the data stream or by adding listeners to them, please be aware there will be the intermediate value and the final value. The intermediate value is computed by the numerical algorithm, then the final value is computed via a post-processing "current conservation" step, which often just changes the values slightly.

[arouinfar]

Thanks for writing that up @samreid! I think it makes sense to keep all sim-specific documentation in a single file, so I've added your comment to #865. The primary purpose of examples.md will remain the same (list of example customizations) but I think there will be situations where we want to provide additional documentation.

We can continue to refer to the doc as "Examples" but we should probably revisit the description on the wrapper index page and the references to it in the PhET-iO Guide. How does that sound @samreid? If you agree, we can open up issues in the appropriate repos.

I think the last TODO for this issue is to set currentProperty, powerDissipatedProperty and powerGeneratedProperty to phetioHighFrequency: true.

[samreid]

@matthew-blackman and I set those to phetioHighFrequency: true and also saw that Vertex positionProperty was spamming the console, so we marked it as high frequency too. We both reviewed and saw it is working properly, so this issue seems good to close. Thanks!

We can continue to refer to the doc as "Examples" but we should probably revisit the description on the wrapper index page and the references to it in the PhET-iO Guide. How does that sound @samreid? If you agree, we can open up issues in the appropriate repos.

The text currently says "Provides instructions and the specific phetioIDs for customizing the simulation." which I don't think is too bad. In this case, it is kind of about customizing how you use the data stream. Anyways, please open a side issue or let me know if that should be changed for this release.

[arouinfar]

Thanks @samreid @matthew-blackman! The changes look good in master.

I'll leave this self-assigned to open up issues related to the Examples doc, and then we're good to close.

[arouinfar]

I agree with @samreid, I think the wrapper index page is sufficiently general. I made a minor wording change to the PhET-iO Guide in https://github.com/phetsims/phet-io-sim-specific/issues/19. There is no need to publish any MR or cherry-pick into existing RC's.