Current sign doesn't reset to positive when multiple circuits are on same screen

[Nancy-Salpepi]

Test device MacBook Air (m1 chip)

Operating System 12.0.1

Browser safari

Problem description For https://github.com/phetsims/qa/issues/748

I noticed that whenever I opened and then closed the switch, it reset the current value back to positive (if negative). However, when there is more than one circuit on the same screen, both displaying negative currents, opening/closing the switch does not lead to a reset.

Steps to reproduce

1. Turn on 'Signed' for ammeter readout in the Options menu
2. On any screen--Make 2 simple circuits on the same screen with a battery, switch and lightbulb.
3. Attach ammeters to both
4. Close switches in both circuits
5. Flip batteries in both so that current reading are negative
6. Open and close switch in either circuit

Visuals

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Circuit Construction Kit: AC‬ URL: https://phet-dev.colorado.edu/html/circuit-construction-kit-ac/1.0.0-rc.1/phet/circuit-construction-kit-ac_all_phet.html Version: 1.0.0-rc.1 2021-12-03 15:30:26 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/15.1 Safari/605.1.15 Language: en-US Window: 1049x698 Pixel Ratio: 2/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}

[Nancy-Salpepi]

Also, if the current sign was positive for one circuit, it can change to negative when the switch is opened/closed, if the other circuit shows a negative current.

Steps:

1. Create 2 simple circuits with a switch, battery and lightbulb.
2. Turn on 'Signed' for ammeter readout in the Options menu
3. Attach an ammeter to each circuit
4. Close both switches
5. Flip the battery in one circuit--its current will have a negative sign
6. Open and close the switch for the second circuit--its current will also change to negative.

[samreid]

@arouinfar and I discussed, and would like to investigate this. This kind of breaks the usability of the signed readout, because it displays an incorrect physical change. I'll spend up to 4 hours investigating.

[samreid]

There is a chance https://github.com/phetsims/circuit-construction-kit-common/issues/793 may fix this.

[samreid]

793 does not fix this. It is being caused by CircuitElement.determineSense which has this code:

If a CircuitElement's sense is unspecified, but some other circuit element (call it the root circuit element) sense is specified, then choose a sense so the overall sign matches the root. The problem in this case is the root is not connected to the circuit element. I considered writing a graph search to find all the connected CircuitElements, which would solve the case-at-hand, but realized it would fail for this case, where they are connected but should be acting independent:

I'm wondering if what we really need is a graph search that applies current senses as it traverses, but again how will it handle the circuit in the image?

Another solution we discussed is that maybe opening a switch wouldn't clear the sense for elements in that loop. That way when you close the switch everything has the same sense as before. But what if the user opens as switch, then deletes some elements, connects different elements, changes the circuit topology, and closes the switch?

Brainstorming (maybe we already tried things like this):

• We could have a circuit element permanently remember its sense, unless it is fully disconnected? Here is a patch that tries that idea:

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 02987d20ace9c6ddc16b6bf484839a026c7cc988)
+++ b/js/model/CircuitElement.ts    (date 1638945560151)
@@ -253,7 +253,8 @@
     const current = this.currentProperty.value;

     // Disconnected circuits have a current of exactly 0, so we can use that to determine when to clear the sense
-    const isReadyToClear = current === 0;
+    const isReadyToClear = circuit.getNeighborCircuitElements( this.startVertexProperty.value ).length === 1 &&
+                           circuit.getNeighborCircuitElements( this.endVertexProperty.value ).length === 1;

     if ( isReadyToClear ) {

For reference, we decided on the existing "current sense is cleared when the current is zero" rule in https://github.com/phetsims/circuit-construction-kit-common/issues/678, and it has other considerations for things that should be in phase.

UPDATE: After reviewing the case in https://github.com/phetsims/circuit-construction-kit-common/issues/678#issuecomment-920345675 I understand the behavior better. We designed the behavior so that one AC source will match phase with an AC source in a completely separate circuit. The circuit discovered in https://github.com/phetsims/circuit-construction-kit-common/issues/800#issue-1072599608 is just like that "multiple AC sources case"--just imagine the battery (whose polarity we flipped manually) is just a "step function AC source" -- the other circuit is just trying to match it.

@arouinfar or @kathy-phet can you please advise for this issue? It's the last issue from the RC that doesn't have a proposed cherry-pick.

[samreid]

From slack:

I’m about to commit the proposed fix, but I have more questions about corner cases. Want to chat again? For instance, if you connect a battery resistor circuit, then flip the battery polarity, then disconnect the battery and delete everything except the battery, then reconnect the battery, it will show initial current readout as negative. Is that OK? But if you put the battery back in the toolbox and take it out, it is technically a new battery so then it would read positive. Want to add a rule that if a circuit element has no neighbors then it also resets its sense?

Also, my proposed implementation leverages existing code searchVertices and has unnecessary overhead. We could write an optimized version if we are concerned about performance. This runs on every circuit element with 0 current on every frame.

Here's the current patch:

```diff 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 02987d20ace9c6ddc16b6bf484839a026c7cc988) +++ b/js/model/CircuitElement.ts (date 1638982165794) @@ -25,6 +25,7 @@ import CurrentSense from './CurrentSense.js'; import Vertex from './Vertex.js'; import IReadOnlyProperty, { PropertyLinkListener } from '../../../axon/js/IReadOnlyProperty.js'; +import ACVoltage from './ACVoltage.js'; // variables let index = 0; @@ -249,16 +250,26 @@ * @public */ determineSense( time: number, circuit: Circuit ) { - const current = this.currentProperty.value; // Disconnected circuits have a current of exactly 0, so we can use that to determine when to clear the sense - const isReadyToClear = current === 0; + // Only clear out if current==0 and it is in a loop with an ac source that has nonzero voltage + if ( current === 0 ) { + let foundAC = false; + circuit.searchVertices( this.startVertexProperty.value, ( a: Vertex, c: CircuitElement, b: Vertex ) => { + if ( c instanceof ACVoltage ) { + foundAC = true; + } - if ( isReadyToClear ) { + // If we have found an AC source, terminate the search for performance reasons + return !foundAC; + } ); + + if ( foundAC ) { - // Reset directionality, and take new directionality when current flows again - this.currentSenseProperty.value = 'unspecified'; + // Reset directionality, and take new directionality when current flows again + this.currentSenseProperty.value = 'unspecified'; + } } else if ( this.currentSenseProperty.value === 'unspecified' ) { ```

[samreid]

Latest patch from second discussion with @kathy-phet and @arouinfar where we only use ac sources as references:

```diff 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 02987d20ace9c6ddc16b6bf484839a026c7cc988) +++ b/js/model/CircuitElement.ts (date 1638994742482) @@ -25,6 +25,7 @@ import CurrentSense from './CurrentSense.js'; import Vertex from './Vertex.js'; import IReadOnlyProperty, { PropertyLinkListener } from '../../../axon/js/IReadOnlyProperty.js'; +import ACVoltage from './ACVoltage.js'; // variables let index = 0; @@ -249,29 +250,42 @@ * @public */ determineSense( time: number, circuit: Circuit ) { - const current = this.currentProperty.value; // Disconnected circuits have a current of exactly 0, so we can use that to determine when to clear the sense - const isReadyToClear = current === 0; + // Only clear out if current==0 and it is in a loop with an ac source that has nonzero voltage + if ( current === 0 ) { + + let foundAC = false; + circuit.searchVertices( this.startVertexProperty.value, ( a: Vertex, c: CircuitElement, b: Vertex ) => { + if ( c instanceof ACVoltage ) { + foundAC = true; + } - if ( isReadyToClear ) { + // If we have found an AC source, terminate the search for performance reasons + return !foundAC; + } ); + + if ( foundAC ) { - // Reset directionality, and take new directionality when current flows again - this.currentSenseProperty.value = 'unspecified'; + // Reset directionality, and take new directionality when current flows again + this.currentSenseProperty.value = 'unspecified'; + } } else if ( this.currentSenseProperty.value === 'unspecified' ) { // If there are other circuit elements, match with them. - const otherCircuitElements = circuit.circuitElements.filter( c => c !== this && c.currentSenseProperty.value !== 'unspecified' ); - if ( otherCircuitElements.length === 0 ) { + const otherSpecifiedACSources = circuit.circuitElements.filter( c => c !== this && + c.currentSenseProperty.value !== 'unspecified' && + c instanceof ACVoltage ); + if ( otherSpecifiedACSources.length === 0 ) { // If this is the first circuit element, choose the current sense so the initial readout is positive this.currentSenseProperty.value = getSenseForPositive( current ); } else { - const rootElement = otherCircuitElements[ 0 ]; + const rootElement = otherSpecifiedACSources[ 0 ]; const desiredSign = rootElement.currentProperty.value >= 0 && rootElement.currentSenseProperty.value === 'forward' ? 'positive' : rootElement.currentProperty.value >= 0 && rootElement.currentSenseProperty.value === 'backward' ? 'negative' : ```

[kathy-phet]

@samreid - I would like to be able to play with a dev version that follows this rule and see what breaks?

Brainstorming (maybe we already tried things like this): We could have a circuit element [starts "unspecified" but then] permanently remember its sense, unless it is fully disconnected? Here is a patch that tries that idea:

Starting with that rule only. Then I'm wondering if that in combination with "if unspecificied, use any other "AC sources" if they exist to set the sense", might do the trick.

If you could temporarily make a commit like this and then publish a dev version, so we can play with it? Unless @arouinfar sees a fatal flaw just by considering this option.

[samreid]

I combined the "We could have a circuit element [starts "unspecified" but then] permanently remember its sense, unless it is fully disconnected?" patch with the "use any other "AC sources for reference" idea in this patch:

```diff 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 02987d20ace9c6ddc16b6bf484839a026c7cc988) +++ b/js/model/CircuitElement.ts (date 1639070213724) @@ -25,6 +25,7 @@ import CurrentSense from './CurrentSense.js'; import Vertex from './Vertex.js'; import IReadOnlyProperty, { PropertyLinkListener } from '../../../axon/js/IReadOnlyProperty.js'; +import ACVoltage from './ACVoltage.js'; // variables let index = 0; @@ -252,8 +253,8 @@ const current = this.currentProperty.value; - // Disconnected circuits have a current of exactly 0, so we can use that to determine when to clear the sense - const isReadyToClear = current === 0; + const isReadyToClear = circuit.getNeighborCircuitElements( this.startVertexProperty.value ).length === 1 && + circuit.getNeighborCircuitElements( this.endVertexProperty.value ).length === 1; if ( isReadyToClear ) { @@ -263,7 +264,9 @@ else if ( this.currentSenseProperty.value === 'unspecified' ) { // If there are other circuit elements, match with them. - const otherCircuitElements = circuit.circuitElements.filter( c => c !== this && c.currentSenseProperty.value !== 'unspecified' ); + const otherCircuitElements = circuit.circuitElements.filter( c => c !== this && + c.currentSenseProperty.value !== 'unspecified' && + c instanceof ACVoltage ); if ( otherCircuitElements.length === 0 ) { // If this is the first circuit element, choose the current sense so the initial readout is positive ```

Initial setups will work ok, but here's a case that will fail it. If you connect circuits like this, the currents will be in phase:

However, if you cut out a wire from one loop, and get a new wire from the toolbox, the new wire will be "unspecified" and hence determine its sense when connected. That means depending on when you connect it, it can have an incompatible sense with the rest of the circuit:

I took this picture after moving the ammeter probe between the wire and AC Source (after grafting on a new wire at the wrong time). Notice the discontinuity.

[samreid]

@arouinfar and I found this patch to be promising:

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 02987d20ace9c6ddc16b6bf484839a026c7cc988)
+++ b/js/model/CircuitElement.ts    (date 1639071399732)
@@ -25,6 +25,7 @@
 import CurrentSense from './CurrentSense.js';
 import Vertex from './Vertex.js';
 import IReadOnlyProperty, { PropertyLinkListener } from '../../../axon/js/IReadOnlyProperty.js';
+import ACVoltage from './ACVoltage.js';

 // variables
 let index = 0;
@@ -249,21 +250,33 @@
    * @public
    */
   determineSense( time: number, circuit: Circuit ) {
-
     const current = this.currentProperty.value;

     // Disconnected circuits have a current of exactly 0, so we can use that to determine when to clear the sense
-    const isReadyToClear = current === 0;
+    // Only clear out if current==0 and it is in a loop with an ac source that has nonzero voltage
+    if ( current === 0 ) {
+      let foundAC = false;
+      circuit.searchVertices( this.startVertexProperty.value, ( a: Vertex, c: CircuitElement, b: Vertex ) => {
+        if ( c instanceof ACVoltage ) {
+          foundAC = true;
+        }

-    if ( isReadyToClear ) {
+        // If we have found an AC source, terminate the search for performance reasons
+        return !foundAC;
+      } );
+
+      if ( foundAC ) {

-      // Reset directionality, and take new directionality when current flows again
-      this.currentSenseProperty.value = 'unspecified';
+        // Reset directionality, and take new directionality when current flows again
+        this.currentSenseProperty.value = 'unspecified';
+      }
     }
     else if ( this.currentSenseProperty.value === 'unspecified' ) {

       // If there are other circuit elements, match with them.
-      const otherCircuitElements = circuit.circuitElements.filter( c => c !== this && c.currentSenseProperty.value !== 'unspecified' );
+      const otherCircuitElements = circuit.circuitElements.filter( c => c !== this &&
+                                                                        c.currentSenseProperty.value !== 'unspecified' &&
+                                                                        c instanceof ACVoltage );
       if ( otherCircuitElements.length === 0 ) {

         // If this is the first circuit element, choose the current sense so the initial readout is positive

We saw one discontinutity but cannot reproduce it.

[samreid]

@arouinfar and I tried this strategy which looks at locally connected circuit elements to choose the polarity. However, even adjacent connected elements may have an inconsistent polarity which would lead to discontinuities:

```diff 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 02987d20ace9c6ddc16b6bf484839a026c7cc988) +++ b/js/model/CircuitElement.ts (date 1639074592635) @@ -25,6 +25,7 @@ import CurrentSense from './CurrentSense.js'; import Vertex from './Vertex.js'; import IReadOnlyProperty, { PropertyLinkListener } from '../../../axon/js/IReadOnlyProperty.js'; +import ACVoltage from './ACVoltage.js'; // variables let index = 0; @@ -253,7 +254,14 @@ const current = this.currentProperty.value; // Disconnected circuits have a current of exactly 0, so we can use that to determine when to clear the sense - const isReadyToClear = current === 0; + const isReadyToClear = circuit.getNeighborCircuitElements( this.startVertexProperty.value ).length === 1 && + circuit.getNeighborCircuitElements( this.endVertexProperty.value ).length === 1; + + const list: CircuitElement[] = []; + circuit.searchVertices( this.startVertexProperty.value, ( a: Vertex, c: CircuitElement, b: Vertex ) => { + list.push( c ); + return true; + } ); if ( isReadyToClear ) { @@ -262,17 +270,28 @@ } else if ( this.currentSenseProperty.value === 'unspecified' ) { - // If there are other circuit elements, match with them. - const otherCircuitElements = circuit.circuitElements.filter( c => c !== this && c.currentSenseProperty.value !== 'unspecified' ); - if ( otherCircuitElements.length === 0 ) { + let rootElement = null; + const otherConnectedCircuitElements = list.filter( c => c !== this && c.currentSenseProperty.value !== 'unspecified' ); + if ( otherConnectedCircuitElements.length > 0 ) { + rootElement = otherConnectedCircuitElements[ 0 ]; + } + else { + + // If there are other circuit elements, match with them. + const otherCircuitElements = circuit.circuitElements.filter( c => c !== this && c.currentSenseProperty.value !== 'unspecified' && + c instanceof ACVoltage ); + + if ( rootElement === null && otherCircuitElements.length > 0 ) { + rootElement = otherCircuitElements[ 0 ]; + } + } + + if ( !rootElement ) { // If this is the first circuit element, choose the current sense so the initial readout is positive this.currentSenseProperty.value = getSenseForPositive( current ); } else { - - const rootElement = otherCircuitElements[ 0 ]; - const desiredSign = rootElement.currentProperty.value >= 0 && rootElement.currentSenseProperty.value === 'forward' ? 'positive' : rootElement.currentProperty.value >= 0 && rootElement.currentSenseProperty.value === 'backward' ? 'negative' : rootElement.currentProperty.value < 0 && rootElement.currentSenseProperty.value === 'forward' ? 'negative' : ```

https://user-images.githubusercontent.com/679486/145457122-9345a05f-2c5e-4ccd-a1c4-8132f45aa832.mov

We think we may be able to get this strategy working if we can create a rule that decides which adjacent circuit element to take the polarity from, but we have no idea how to write a rule like that.

[samreid]

We considered this case, and we don't think we can guarantee consistency at the junction:

However, each loop needs to be self-consistent.

[samreid]

We would like to make sure the current readouts are consistent throughout this circuit:

[samreid]

We are considering looking at neighbors to see if one is specified. @kathy-phet suggested that going from the high voltage side, then choosing the "highest voltage" neighbor would be a consistent way to assign senses.

[samreid]

Current status:

```diff Index: js/view/CircuitDebugLayer.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/CircuitDebugLayer.ts b/js/view/CircuitDebugLayer.ts --- a/js/view/CircuitDebugLayer.ts (revision 02987d20ace9c6ddc16b6bf484839a026c7cc988) +++ b/js/view/CircuitDebugLayer.ts (date 1639085657293) @@ -72,7 +72,7 @@ } ); this.addChild( senseNode ); - const textNode = new Text( circuitElement.currentProperty.value ); //eslint-disable-line + const textNode = new Text( circuitElement.currentProperty.value.toFixed( 4 ) ); //eslint-disable-line const panel = new Panel( textNode, { center: arrowNode.center, fill: circuitElement.currentSenseProperty.value === 'forward' ? 'green' : 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 02987d20ace9c6ddc16b6bf484839a026c7cc988) +++ b/js/model/CircuitElement.ts (date 1639084634812) @@ -25,6 +25,7 @@ import CurrentSense from './CurrentSense.js'; import Vertex from './Vertex.js'; import IReadOnlyProperty, { PropertyLinkListener } from '../../../axon/js/IReadOnlyProperty.js'; +import ACVoltage from './ACVoltage.js'; // variables let index = 0; @@ -248,44 +249,74 @@ * @param {Circuit} circuit * @public */ - determineSense( time: number, circuit: Circuit ) { - - const current = this.currentProperty.value; - - // Disconnected circuits have a current of exactly 0, so we can use that to determine when to clear the sense - const isReadyToClear = current === 0; - - if ( isReadyToClear ) { - - // Reset directionality, and take new directionality when current flows again - this.currentSenseProperty.value = 'unspecified'; - } - else if ( this.currentSenseProperty.value === 'unspecified' ) { - - // If there are other circuit elements, match with them. - const otherCircuitElements = circuit.circuitElements.filter( c => c !== this && c.currentSenseProperty.value !== 'unspecified' ); - if ( otherCircuitElements.length === 0 ) { - - // If this is the first circuit element, choose the current sense so the initial readout is positive - this.currentSenseProperty.value = getSenseForPositive( current ); - } - else { - - const rootElement = otherCircuitElements[ 0 ]; - - const desiredSign = rootElement.currentProperty.value >= 0 && rootElement.currentSenseProperty.value === 'forward' ? 'positive' : - rootElement.currentProperty.value >= 0 && rootElement.currentSenseProperty.value === 'backward' ? 'negative' : - rootElement.currentProperty.value < 0 && rootElement.currentSenseProperty.value === 'forward' ? 'negative' : - rootElement.currentProperty.value < 0 && rootElement.currentSenseProperty.value === 'backward' ? 'positive' : - 'error'; - - assert && assert( desiredSign !== 'error' ); - this.currentSenseProperty.value = desiredSign === 'positive' ? - getSenseForPositive( current ) : - getSenseForNegative( current ); - } - } - } + // determineSense( time: number, circuit: Circuit ) { + // const current = this.currentProperty.value; + // + // // Disconnected circuits have a current of exactly 0, so we can use that to determine when to clear the sense + // // Only clear out if current==0 and it is in a loop with an ac source that has nonzero voltage + // if ( current === 0 ) { + // // let foundAC = false; + // // circuit.searchVertices( this.startVertexProperty.value, ( a: Vertex, c: CircuitElement, b: Vertex ) => { + // // if ( c instanceof ACVoltage ) { + // // foundAC = true; + // // } + // // + // // // If we have found an AC source, terminate the search for performance reasons + // // return !foundAC; + // // } ); + // // + // // if ( foundAC ) { + // + // // Reset directionality, and take new directionality when current flows again + // this.currentSenseProperty.value = 'unspecified'; + // // } + // } + // else if ( this.currentSenseProperty.value === 'unspecified' ) { + // + // // choose the high voltage side + // const highVoltageVertex = this.startVertexProperty.value.voltageProperty.value > this.endVertexProperty.value.voltageProperty.value ? + // this.startVertexProperty.value : this.endVertexProperty.value; + // const v = highVoltageVertex.voltageProperty.value; + // + // const neighborsOnTheHighSide = circuit.getNeighborCircuitElements( highVoltageVertex ).filter( c => c !== this ); + // + // let root: CircuitElement | null = null; + // + // for ( let i = 0; i < neighborsOnTheHighSide.length; i++ ) { + // const neighbor = neighborsOnTheHighSide[ i ]; + // const oppositeVertex = neighbor.getOppositeVertex( highVoltageVertex ); + // const oppositeVoltage = oppositeVertex.voltageProperty.value; + // if ( oppositeVoltage > v && neighbor.currentSenseProperty.value !== 'unspecified' ) { + // root = neighbor; + // break; + // } + // } + // + // // If there are other circuit elements, match with them. + // const otherCircuitElements = list.filter( c => c !== this && + // c.currentSenseProperty.value !== 'unspecified' ); + // if ( otherCircuitElements.length === 0 ) { + // + // // If this is the first circuit element, choose the current sense so the initial readout is positive + // this.currentSenseProperty.value = getSenseForPositive( current ); + // } + // else { + // + // const rootElement = otherCircuitElements[ 0 ]; + // + // const desiredSign = rootElement.currentProperty.value >= 0 && rootElement.currentSenseProperty.value === 'forward' ? 'positive' : + // rootElement.currentProperty.value >= 0 && rootElement.currentSenseProperty.value === 'backward' ? 'negative' : + // rootElement.currentProperty.value < 0 && rootElement.currentSenseProperty.value === 'forward' ? 'negative' : + // rootElement.currentProperty.value < 0 && rootElement.currentSenseProperty.value === 'backward' ? 'positive' : + // 'error'; + // + // assert && assert( desiredSign !== 'error' ); + // this.currentSenseProperty.value = desiredSign === 'positive' ? + // getSenseForPositive( current ) : + // getSenseForNegative( current ); + // } + // } + // } /** * Steps forward in time @@ -497,12 +528,12 @@ } } ); -const getSenseForPositive = ( current: number ) => current < 0 ? 'backward' : - current > 0 ? 'forward' : - 'unspecified'; -const getSenseForNegative = ( current: number ) => current < 0 ? 'forward' : - current > 0 ? 'backward' : - 'unspecified'; +// const getSenseForPositive = ( current: number ) => current < 0 ? 'backward' : +// current > 0 ? 'forward' : +// 'unspecified'; +// const getSenseForNegative = ( current: number ) => current < 0 ? 'forward' : +// current > 0 ? 'backward' : +// 'unspecified'; circuitConstructionKitCommon.register( 'CircuitElement', CircuitElement ); export type { CircuitElementOptions }; 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 02987d20ace9c6ddc16b6bf484839a026c7cc988) +++ b/js/model/Circuit.ts (date 1639089291358) @@ -976,8 +976,12 @@ this.circuitChangedEmitter.emit(); } + this.determineSenses(); + // const allVerticesToSearch = this.circuitElements[ 0 ]. + // this.searchVertices( this.circuitElements[ 0 ].startVertexProperty.value ); + // Update the sense of each CircuitElement after we determined the current values - stepElements.forEach( element => element.determineSense( this.timeProperty.value, this ) ); + // stepElements.forEach( element => element.determineSense( this.timeProperty.value, this ) ); // Move the charges. Do this after the circuit has been solved so the conventional current will have the correct // current values. @@ -1071,6 +1075,81 @@ return this.searchVertices( vertex, trueFunction ); } + determineSenses() { + + // Disconnected circuit elements forget their sense + this.circuitElements.forEach( c => { + if ( c.currentProperty.value === 0.0 ) { + c.currentSenseProperty.value = 'unspecified'; + } + } ); + + const getSenseForPositive = ( current: number ) => current < 0 ? 'backward' : + current > 0 ? 'forward' : + 'unspecified'; + const getSenseForNegative = ( current: number ) => current < 0 ? 'forward' : + current > 0 ? 'backward' : + 'unspecified'; + + const circuitElementsWithSenses = this.circuitElements.filter( c => c.currentSenseProperty.value !== 'unspecified' ); + if ( circuitElementsWithSenses.length === 0 ) { + this.circuitElements.forEach( c => {c.currentSenseProperty.value = getSenseForPositive( c.currentProperty.value );} ); + } + else { + + // launch searches from circuit elements with senses + const toVisit: Vertex[] = []; + circuitElementsWithSenses.forEach( c => { + toVisit.push( c.startVertexProperty.value ); + toVisit.push( c.endVertexProperty.value ); + } ); + + const visited: Vertex[] = []; + while ( toVisit.length > 0 ) { + const currentVertex = toVisit.pop() as Vertex; + if ( !visited.includes( currentVertex ) ) { + const neighborCircuitElements = this.getNeighborCircuitElements( currentVertex ); + for ( let i = 0; i < neighborCircuitElements.length; i++ ) { + const circuitElement = neighborCircuitElements[ i ]; + const neighborVertex = circuitElement.getOppositeVertex( currentVertex ); + if ( !visited.includes( neighborVertex ) ) { + + // choose sense from a neighbor + const specifiedNeighbors = neighborCircuitElements.filter( c => c !== circuitElement && c.currentSenseProperty.value !== 'unspecified' ); + + if ( circuitElement.currentSenseProperty.value === 'unspecified' && Math.abs( circuitElement.currentProperty.value ) > 0 && specifiedNeighbors.length > 0 ) { + const specified = specifiedNeighbors[ 0 ]; + + const desiredSign = specified.currentProperty.value >= 0 && specified.currentSenseProperty.value === 'forward' ? 'positive' : + specified.currentProperty.value >= 0 && specified.currentSenseProperty.value === 'backward' ? 'negative' : + specified.currentProperty.value < 0 && specified.currentSenseProperty.value === 'forward' ? 'negative' : + specified.currentProperty.value < 0 && specified.currentSenseProperty.value === 'backward' ? 'positive' : + 'error'; + + assert && assert( desiredSign !== 'error' ); + circuitElement.currentSenseProperty.value = desiredSign === 'positive' ? + getSenseForPositive( circuitElement.currentProperty.value ) : + getSenseForNegative( circuitElement.currentProperty.value ); + + // const highVoltageVertex = circuitElement.startVertexProperty.value.voltageProperty.value > circuitElement.endVertexProperty.value.voltageProperty.value ? + // circuitElement.startVertexProperty.value : circuitElement.endVertexProperty.value; + // const v = highVoltageVertex.voltageProperty.value; + // + // const neighborsOnTheHighSide = this.getNeighborCircuitElements( highVoltageVertex ).filter( c => c !== circuitElement ); + } + + toVisit.push( neighborVertex ); + } + } + } + if ( visited.indexOf( currentVertex ) < 0 ) { + visited.push( currentVertex ); + } + } + // after searching everything, if a circuit element hasn't been assigned a sense, it can just go positive. + } + } + /** * Find the subgraph where all vertices are connected, given the list of traversible circuit elements. * There are a few other ad-hoc graph searches around, such as isInLoop and in LinearTransientAnalysis ```

[samreid]

I rewrote the algorithm in Circuit.ts, and made it more holistic. Let me describe the algorithm at the moment. This happens once per step

Circuit elements with no current forget their sense
WHILE (true)
  Run a depth-first-search starting from all vertices of circuit elements with known sense, 
     assigning sense from neighbors during the traversal
  IF no remaining circuit elements need a sense
      EXIT WHILE LOOP
  ELSE 
      IF an AC source can match to another AC source
         THEN assign sense based on matching with the other AC source, and repeat the WHILE loop
      IF NOT
          THEN Choose one unspecified circuit element to have positive current, and repeat the WHILE loop

I planned for the first draft to have the "assigning sense from neighbors during the traversal" follow the "take the voltage from the high voltage side's highest voltage neighbor", but I haven't reproduced a case where this is necessary yet. I tested all of the cases we looked at today (but I haven't tried them in all possible circuit construction orders). There may be one but I haven't found it yet. I left a placeholder for if/when we need to add that logic. But I also noted that there are questions about how to apply that logic:

• What if the high voltage side only has unspecified elements attached to it? Can we look at the low voltage side, and take a circuit element with the lowest value, then take the opposite?
• What if the high voltage side has 2 neighbors, one with 5V and one with 6V, but the 6V neighbor is 'unspecified'. Should we take the sense from the 5V neighbor or wait and see if the 6V neighbor can be filled in by the rest of the search?

Based on these complex questions and the fact that I haven't yet isolated a case where this complexity is necessary, I decided to commit what I have. We should continue testing and see if we can break it. Once we can break it reliably, we can try to adjust the "which neighbor to take sense from" based on the high voltage strategy.

[kathy-phet]

With this circuit I was able to get the phases of the AC circuits out of sync, if I opened and closed the switch on the right and timed it correctly to be out of phase. But that is because the AC source on the right gets its "sense" from the existing sense of the battery loop in the middle.

This isn't technically broken ... just the AC is out of phase by 180 degree. I don't see a way around this ... there is an existing sense definition by the middle loop, and we are matching that preferentially.

I tested a bunch of circuits and it seems to be working great overall!

I can test it more tomorrow. @arouinfar @ariel-phet - Can you test this solution as you have time? @samreid - Can you make a "ready-to-test" test issue for QA to see if they can bang on this signs solution to verify it seems good, and doesn't cause performance issues in unexpected ways? Before we do any sort of cherry pick into the RC.

[kathy-phet]

When you are testing, know that expected behavior is ...

• if a stand alone circuit has no current in it at all (either by breaking the path or opening a switch), it will always choose positive for the current when reconnected if DC, or match the sense of an existing AC source if AC.

[kathy-phet]

@samreid - How "expensive" is this algorithm, performance wise?

[samreid]

Sure, I'll post a dev version soon for QA testing. I may want to adjust some things first (there are 3 TODOs in the code to consider). Also, I created the circuit from 3 comments ago and looked at the "steady state" performance cost (while things are running, not just the moment something is connected), and it seems on the low side, significantly lower than the charge animator, or the modified nodal analysis.

My current feeling is that we should be able to construct a failing case, because during traversal, the rule is currently: of the "upstream" neighbors with a defined sense, choose the one with the lowest index. "upstream" means the direction the search came from. So can't we create a case where there are multiple upstream "sensed" elements, but not all of them are compatible for determining the sense? Or maybe we can convince ourselves any would be OK? I'm still uncertain about that part.

[kathy-phet]

A circuit this complicated is able to get some discontinuities and change the way it behaves based on timing of closing the switch. The parallel branches here have different current signs now, even though flow is in same direction. But overall, for simple circuits, this is getting much better behaved. Haven't tried to see if I can mess it up with just DC.

[kathy-phet]

I made this circuit ...

It's easy to get the top current to switch relative to the one on the left, because I can just flip the battery in that bottom loop, and then the top loop matches the sense in that "lowest z-order" wire.

However, in this case I cannot think of a possibility that would keep them all "in sync" or that they should even be "in sync". We have 3 circuits and each battery is controllable independently in terms of its sign.

[kathy-phet]

@samreid @arouinfar - While the new algorithm cannot create a situation that is "in sync" for all circumstances, it seems pretty robust for common simple circuits ... one voltage source, parallel and series situations.

I would favor moving forward with a dev test and see if the QA team finds any simple cases which seem incorrect.

And ask Ariel to bang on it too to see if he finds any simple cases where signs seem to be messed up.

With the current algorithm, I do not think there is any way to get a sign to flip and show a discontinuity without crossing a multi-pronged node. So I think that is an improvement for sure.

[samreid]

In the commit: renaming, refactoring, simplification, performance improvements. Should not change overall behavior, but should be better for maintainability.

[samreid]

In testing a pair of AC voltage circuits, I observed the following:

Connecting at arbitrary time, they are in phase as desired:

Then, changing the phase puts them out of phase, as expected:

However, leaving the "phase shift" controls out of phase, then open and closing a switch puts them back in phase, because the sense determining algorithm doesn't look at the "phase shift" controls. Hopefully that is OK

[arouinfar]

However, leaving the "phase shift" controls out of phase, then open and closing a switch puts them back in phase, because the sense determining algorithm doesn't look at the "phase shift" controls. Hopefully that is OK

This is definitely fine for 1.0 and easy enough to document in the Teacher Tips. Something we could consider for the future is adding a way to manually clear the phase (perhaps a button in the edit panel) to re-sync things up.

[arouinfar]

I would favor moving forward with a dev test and see if the QA team finds any simple cases which seem incorrect. And ask Ariel to bang on it too to see if he finds any simple cases where signs seem to be messed up.

This sounds like a great plan. I would only consider sign errors blocking if they show up in circuits that would appear in a typical classroom activity. Generally, it seems like issues arise when there are voltage sources in separate but strangely connected loops.

[samreid]

For clarification, @kathy-phet and I decided to do this testing with RC.2 instead of a dev version.

[kathy-phet]

However, leaving the "phase shift" controls out of phase, then open and closing a switch puts them back in phase, because the sense determining algorithm doesn't look at the "phase shift" controls. Hopefully that is OK

This is definitely fine for 1.0 and easy enough to document in the Teacher Tips. Something we could consider for the future is adding a way to manually clear the phase (perhaps a button in the edit panel) to re-sync things up.

@samreid - Can you make a new issue for this? So we can fix it when we do PhET-iO add. I think its fixable.

[ariel-phet]

@kathy-phet @samreid @arouinfar here is a simple case in 1.0.0-rc.2 in which I can easily confuse the signs using identical DC circuits.

1. Build two identical circuits using the default components using a switch, battery and light bulb
2. In one of the circuits use the "flip battery" control - at first the current will read properly as the opposite sign
3. Open the switch in the flipped battery circuit, then close it again - the current will be still flowing the opposite direction of the first circuit (as it should) but the sign of the current will be incorrect in comparison

https://user-images.githubusercontent.com/4051174/145844382-44333f57-64b1-454e-8885-476801c5b182.mp4

[kathy-phet]

@ariel-phet - This is the actually intended behavior at the moment. We tried various versions of the "don't forget your current sense" unless you are disconnected from the circuit completely, but could not find a robust solution there. So we had to fall back to, "if you have no current, forget your sense, and then if your whole circuit has no current, pick positive.".

Amy will explain the "rule" in the Teacher Tips. It's an explainable rule, but is indeed why keeping it behind a query parameter is good.

We can keep pushing on this issue for the next version, but given all the troubles we saw - which were way more problematic then going positive after opening a switch - we decided to accept this behavior for now.

[ariel-phet]

@kathy-phet thanks for the clarification -- I actually just saw your "intended" behavior comment in the RC testing issue -- I will continue to test with that in mind, I will be careful about connecting and reconnecting circuits to see the behavior is matched. Onward!

[kathy-phet]

Yes, so for instance if you add a parallel branch to the one you made negative above, without opening the switch, it should adopt the negative current that is already in that circuit. If you then open the switch and close the switch it should stay negative, because the parallel branch is still keeping the current sense in "Memory" so to speak.

[ariel-phet]

@samreid @kathy-phet @arouinfar with the understanding of the rule described, things seems self consistent in my testing.

[samreid]

Thanks, I don't have any other work for this issue at the moment, and QA didn't indicate other trouble in RC.2.

We can keep pushing on this issue for the next version

I'll leave this issue open and unmark for the milestone. It's unclear to me how we would do other work for this issue, would be good to discuss before I work on this further.

[samreid]

@arouinfar can you please review and summarize what remains for this issue? Please label for any milestone if appropriate.

[arouinfar]

@samreid there isn't anything left for this issue. The behavior is well-documented in the Teacher Tips and was self-consistent during RC testing. Closing.