Light Bulb stays lit during short circuit

[ariel-phet]

A twitter user pointed out that in the Java version during a short circuit the bulb stays lit (confusing to students). This is incorrect and should be addressed in the HTML5 version. See screenshot below, in this scenario the short circuit electrons are moving quickly and the bulb side are frozen (as they should be) but the bulb is still lit.

@samreid please be aware of this issue as you are dealing with the "fire" code. Not sure if it is still a problem or not.

[samreid]

If I understand correctly, for an ideal battery, the introduction of a short circuit doesn't change the voltage or current through the light bulb. What does change is that the simulation is now running at reduced speed (to prevent strobe effect of electron motion). Hence it appears the electrons aren't moving through the light bulb (and it says speed reduced to <1% normal), but the electrons are moving at the same speed (just the sim is in extreme slow motion). Reassigned to @ariel-phet and @arouinfar for discussion.

[ariel-phet]

@samreid there is a conflict here:

1. an ideal battery would also not catch fire (no internal resistance, so no reason to heat up)

2. We are trying to depict the real world...especially in a basics version, allowing students to see the effects of a short circuit seems an important learning goal (for instance the twitter user specifically pointed out the students were confused the bulb stayed lit in this situation).

Not sure of the correct solution here, but the bulb staying lit in this situation is problematic as it does not reflect what happens in the real world.

[arouinfar]

We had another user write into phethelp about this same issue:

I am an education student at Towson University and I noticed a problem with one of the simulations, the Circuit Construction Kit (DC) only, which shows that the lightbulbs will light up even when there is an alternate circuit where they wouldn't show up. I think this could cause some confusion for students, so I figured I would mention it.

This is something we'll clearly need to address, though I'm conflicted. I'm not convinced that this behavior is wrong (though certainly confusing). In an ideal short, the current through the bulbs would be zero. However the wires do have some internal resistance, so while the current through the bulbs is negligible compared to the current through the short, it is non-zero and the bulbs remain lit.

I think the real confusion arises from the reduction in animation speed. The electrons do not appear to move through the bulbs, though there is still a non-zero current.

We can chat more about it at design meeting.

[arouinfar]

@samreid will play with the animation speed limit so that the electrons through the bulb do not appear frozen.

[samreid]

Here's what happens with the naive approach:

[samreid]

The circuit dynamics assume that no electron can go around 2 corners or more in one time step (and time is slowed down enough to make this so). When we limit the amount the speed can be reduced, (say to 10% or so), the electrons in the short circuit get bunched up. I tried subdividing the dt by 100 and running it 100 times, but this is computationally demanding and still did not have the correct behavior (not subdivided enough). We could look into physics that allows electrons to go around multiple corners per frame, but that will also be computationally demanding. Or we could discuss other ways of addressing this problem. @arouinfar what do you think?

[arouinfar]

@samreid let's discuss during design meeting. I think we'll want to avoid computationally demanding features wherever possible. Perhaps we can make a compromise and turn off the light bulb when shorted?

Suppose these circuits are both in the play area and use the default bulb. The circuit on the right is shorted, so the animation speed is reduced. The electrons through both bulbs will appear frozen, but the light bulb that is connected to the short would no longer light up.

[arouinfar]

Discussed in 6/1/17 design meeting.

For 1.0, the behavior is fine as-is, as it does no harm over the Java version.

To reduce the animation speed of the shorted electrons, we would need to model a current-limited battery (or one that has an internal resistance that scales with the current). We could consider this for 1.1+.

[arouinfar]

I'll also need to document consequences of an ideal battery in the teacher's tips.

[arouinfar]

@KatieWoe reported this in #492. Looks like phethelp is still getting emails about this, so we may want to consider the proposal in https://github.com/phetsims/circuit-construction-kit-common/issues/245#issuecomment-305544556.

For https://github.com/phetsims/QA/issues/311 First pointed out in a phethelp email. When an ammeter is wired in parallel to a circuit to short it out, the lightbulb of the shorted circuit may remain lit. In published version. Steps to reproduce:

1. Build a simple circuit with a bulb, wires, and a battery
2. Add an ammeter parallel to the bulb

Screenshots:

[KatieWoe]

I did a closer look at https://github.com/phetsims/circuit-construction-kit-common/issues/245#issuecomment-487207618 with the point ammeter. I noticed that there is still some current in the lightbulb's path (likely due to small resistance in the wires) and this causes the phenomena. If this is right it may be worth putting in the teacher tips/mentioning when sent into phethelp in the future.

[arouinfar]

Thanks @KatieWoe, this is already mentioned in the teacher tips.

[samreid]

On slack, @ariel-phet said no more development work needs to be done for this issue before the upcoming DC/Fuse RC.

[samreid]

@ariel-phet or @arouinfar should we work on this issue for AC publication?

[ariel-phet]

@arouinfar I think this issue would be worth bringing back to design meeting.

Perhaps another route of addressing the issue would be to add a new component, basically a "short circuit wire", but not mess with the battery being ideal or such. It seems the goal of teachers is to show off what happens in a short circuit, and maybe we could make that goal explicit. I am thinking of something in the shape like a "U" or like "Π" that has control points at the two ends, and anything between the two connection points receives no current. If a battery were shorted with this element, the current if measured on the element would just show the infinity symbol or such. Just a thought of a different potential way to address this goal.

But yes, we should probably have a design meeting to look at some outstanding issues at some point in the near future.

[samreid]

@ariel-phet recommends checking if any element of the circuit exceeds a threshold like 10000A, then anything else on the circuit under a threshold like 100A should be zeroed out.

We would need to detect connected components "in the same circuit" and only apply that rule within that circuit.

@kathy-phet demonstrated a case where circuits are loosely connected, but we can't think of how to program it to treat as separate circuits.

[samreid]

New idea: when the battery current exceeds a threshold, turn up its internal resistance. This will emulate the short.

[samreid]

I'll create two query parameters: one for the battery current threshold and one for the internal resistance and run it past @arouinfar and @ariel-phet. We'll see if that works OK.

[samreid]

I added the two query parameters (shown in the patch)

```js Index: js/CCKCQueryParameters.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/CCKCQueryParameters.js (revision 41716a1641936e2f000de7a69665e1454b4436ef) +++ js/CCKCQueryParameters.js (date 1569805804232) @@ -73,6 +73,16 @@ // see https://github.com/phetsims/circuit-construction-kit-common/issues/432 moreWires: { type: 'flag' + }, + + batteryCurrentThreshold: { + type: 'number', + defaultValue: Number.POSITIVE_INFINITY + }, + + batteryInternalResistanceWhenCurrentThresholdExceeded: { + type: 'number', + defaultValue: 0 } } ); Index: js/model/Battery.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/model/Battery.js (revision 41716a1641936e2f000de7a69665e1454b4436ef) +++ js/model/Battery.js (date 1569806277638) @@ -10,7 +10,9 @@ // modules const CCKCConstants = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/CCKCConstants' ); + const CCKCQueryParameters = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/CCKCQueryParameters' ); const circuitConstructionKitCommon = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/circuitConstructionKitCommon' ); + const DerivedProperty = require( 'AXON/DerivedProperty' ); const Enumeration = require( 'PHET_CORE/Enumeration' ); const FixedCircuitElement = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/model/FixedCircuitElement' ); const NumberProperty = require( 'AXON/NumberProperty' ); @@ -42,7 +44,15 @@ this.voltageProperty = new NumberProperty( options.voltage ); // @public {Property.} the internal resistance of the battery - this.internalResistanceProperty = internalResistanceProperty; + this.internalResistanceProperty = new DerivedProperty( [ internalResistanceProperty, this.currentProperty ], + ( internalResistance, current ) => { + if ( Math.abs( current ) >= CCKCQueryParameters.batteryCurrentThreshold ) { + return CCKCQueryParameters.batteryInternalResistanceWhenCurrentThresholdExceeded; + } + else { + return internalResistance; + } + } ); // @public (read-only) {string} - track which way the battery "button" (plus side) was facing the initial state so // the user can only create a certain number of "left" or "right" batteries from the toolbox. ```

However, in testing, this caused a rapidly oscillating behavior for the battery because of these steps:

The battery current exceeds the threshold ==> this causes the battery resistance to increase ==> this causes the battery current to go below the threshold ==> This causes the current to exceed the threshold.

The only way I can think of around this problem is to run the circuit solver twice. The first time, batteries do not use this threshold criteria and we can compute the currents through all batteries. If any battery exceeds the current threshold, we update their resistances and run the solver again. In order to maintain correct dynamics for inductors and capacitors, we would need to save their initial state before the first solve and roll back to it for the 2nd solve. This is becoming more complicated than we anticipated, so I'll bring it back to design meeting before spending more time.

[kathy-phet]

Sam, if you choose a small enough resistance and a low enough threshhold, this flicker should go away.

Sent from my iPhone

On Sep 30, 2019, at 7:00 AM, Sam Reid notifications@github.com<mailto:notifications@github.com> wrote:

I added the two query parameters (shown in the patch)

Index: js/CCKCQueryParameters.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8

--- js/CCKCQueryParameters.js (revision 41716a1641936e2f000de7a69665e1454b4436ef) +++ js/CCKCQueryParameters.js (date 1569805804232) @@ -73,6 +73,16 @@ // see https://github.com/phetsims/circuit-construction-kit-common/issues/432 moreWires: { type: 'flag'

• },
• batteryCurrentThreshold: {
• type: 'number',
• defaultValue: Number.POSITIVE_INFINITY
• },
• batteryInternalResistanceWhenCurrentThresholdExceeded: {
• type: 'number',
• defaultValue: 0 } } );

Index: js/model/Battery.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8

--- js/model/Battery.js (revision 41716a1641936e2f000de7a69665e1454b4436ef) +++ js/model/Battery.js (date 1569806277638) @@ -10,7 +10,9 @@

// modules const CCKCConstants = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/CCKCConstants' );

• const CCKCQueryParameters = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/CCKCQueryParameters' ); const circuitConstructionKitCommon = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/circuitConstructionKitCommon' );

• const DerivedProperty = require( 'AXON/DerivedProperty' ); const Enumeration = require( 'PHET_CORE/Enumeration' ); const FixedCircuitElement = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/model/FixedCircuitElement' ); const NumberProperty = require( 'AXON/NumberProperty' ); @@ -42,7 +44,15 @@ this.voltageProperty = new NumberProperty( options.voltage );

  // @public {Property.} the internal resistance of the battery

• this.internalResistanceProperty = internalResistanceProperty;
• this.internalResistanceProperty = new DerivedProperty( [ internalResistanceProperty, this.currentProperty ],
• ( internalResistance, current ) => {
• if ( Math.abs( current ) >= CCKCQueryParameters.batteryCurrentThreshold ) {
• return CCKCQueryParameters.batteryInternalResistanceWhenCurrentThresholdExceeded;
• }
• else {
• return internalResistance;
• }

• } );

  // @public (read-only) {string} - track which way the battery "button" (plus side) was facing the initial state so // the user can only create a certain number of "left" or "right" batteries from the toolbox.

However, in testing, this caused a rapidly oscillating behavior for the battery because of these steps:

The battery current exceeds the threshold ==> this causes the battery resistance to increase ==> this causes the battery current to go below the threshold ==> This causes the current to exceed the threshold.

[flicker2]https://user-images.githubusercontent.com/679486/65880970-e414ca00-e34f-11e9-870a-1eff9ad9a919.gif

The only way I can think of around this problem is to run the circuit solver twice. The first time, batteries do not use this threshold criteria and we can compute the currents through all batteries. If any battery exceeds the current threshold, we update their resistances and run the solver again. In order to maintain correct dynamics for inductors and capacitors, we would need to save their initial state before the first solve and roll back to it for the 2nd solve. This is becoming more complicated than we anticipated, so I'll bring it back to design meeting before spending more time.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/phetsims/circuit-construction-kit-common/issues/245?email_source=notifications&email_token=ABG4KZB7LOJQ7ZOUR7RD2DDQMHZ6XA5CNFSM4C7HQ2Q2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD75RS7I#issuecomment-536549757, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABG4KZG2CC6IL5FEJZDQQJLQMHZ6XANCNFSM4C7HQ2QQ.

[samreid]

Thanks! More specifically, if the limited current is under the threshold, there will be flickering. If the limited current remains over the threshold, there will be no flickering. Here's a dev version of the DC simulation we can use for experimenting:

https://phet-dev.colorado.edu/html/circuit-construction-kit-dc/1.2.0-dev.2/phet/circuit-construction-kit-dc_en_phet.html?batteryCurrentThreshold=10&batteryInternalResistanceWhenCurrentThresholdExceeded=10

Note the two query parameters:

• batteryCurrentThreshold
• batteryInternalResistanceWhenCurrentThresholdExceeded

They are set to 10 in the URL above, please specify the values you want to try.

[kathy-phet]

What are the units of these parameters here? that is what does 10 mean physically in units of Amps and ohms?

[samreid]

Yes, batteryCurrentThreshold is in Amps, and batteryInternalResistanceWhenCurrentThresholdExceeded is in Ohms.

[kathy-phet]

can these be set at runtime - through sliders or otherwise - instead of query parameters. It's hard to test with having to reload all the time.

[samreid]

You can set the values dynamically at runtime by using the dev tools. For example, you can paste these commands in the dev tools and the values update in the sim right away without reloading.

phet.circuitConstructionKitCommon.CCKCQueryParameters.batteryCurrentThreshold=20

and

phet.circuitConstructionKitCommon.CCKCQueryParameters.batteryInternalResistanceWhenCurrentThresholdExceeded=7

Please let me know if that helps, or if I should take the time to put a control UI in the sim. If you would like to try a control UI in the sim, please recommend ranges for each parameter.

[samreid]

On second thought, I'm seeing cases where changing this in dev tools may not trigger a recomputation in the sim, since they are buried in a DerivedProperty which only triggers when the current through the battery changes.

@kathy-phet please let me know whether dev tools commands is OK (if I fix it up to update right away) or whether I should build a UI for this experimentation.

[ariel-phet]

@samreid I see the dilemma, We can get close with some parameters, but unfortunately I think we need the current threshold to be pretty high.

Couldn't you have logic that basically goes, once the battery resistance is set to true, it only gets reset to false if there is a change in the matrix describing the circuit? So if the circuit is untouched, the battery resistance remains, but if a wire is stretched, node attached, etc it resolves, but as soon as that condition of battery resistance is set to true again it remains persistent until something in the circuit is perturbed? You must have this information, since iO can load a state of the sim...

[samreid]

The problem is that when the battery resistance is enabled, that counts as a change in the matrix and hence would cause the battery current to be recomputed. It would be very complicated to add infrastructure that lets circuit elements track the reasons behind circuit recomputation (say, so the battery could ignore recomputations caused by its own resistance being enabled). It would be easier and more robust to run the solver twice as described in https://github.com/phetsims/circuit-construction-kit-common/issues/245#issuecomment-536549757

Also, we should work toward a solution that will work in the presence of AC voltage sources, inductors, or capacitors, which require recomputation in each frame.

[ariel-phet]

@samreid understood.

Considering this is a "frame by frame" type calculation, we could in theory then easily implement a resistance that depends on current threshold correct? So there might be a nearly imperceptible flicker at first, but then the current will settle. I assume a linear or such map would be able to be implemented, or a few current thresholds and corresponding resistances? I am pretty sure this will work in some playing I have done. If that is possible to be implemented, let me know, and I will provide you with a mapping.

[ariel-phet]

@samreid actually I might need to think about this some more...what I was thinking will work in many cases...but not in all...

[samreid]

For my reference, here is a verbose patch with extra debugging and console statements:

```diff Index: js/model/DynamicCircuit.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/model/DynamicCircuit.js (revision c2fb8104fa1bee5d1ea013418527929336342db3) +++ js/model/DynamicCircuit.js (date 1571371814530) @@ -153,6 +153,8 @@ */ toMNACircuit( dt ) { + debugger; + const companionBatteries = [];//new ArrayList(); const companionResistors = [];//new ArrayList(); const companionCurrents = [];//new ArrayList(); @@ -192,6 +194,8 @@ const idealBattery = new ModifiedNodalAnalysisCircuitElement( resistiveBattery.nodeId0, newNode, null, resistiveBattery.voltage ); // final LinearCircuitSolver.Battery + console.log( 'resistive battery resistance: ', resistiveBattery.resistance ); + // same type as idealBattery, but treated like a resistor because it goes in the resistor array const idealResistor = new ModifiedNodalAnalysisCircuitElement( newNode, resistiveBattery.nodeId1, null, resistiveBattery.resistance ); // LinearCircuitSolver.Resistor companionBatteries.push( idealBattery ); @@ -200,7 +204,10 @@ //we need to be able to get the current for this component currentCompanions.push( { element: resistiveBattery, - getValueForSolution: solution => idealBattery.currentSolution + getValueForSolution: solution => { + debugger; + return idealBattery.currentSolution; + } } ); } ); @@ -421,7 +428,11 @@ class ResistiveBattery extends ModifiedNodalAnalysisCircuitElement { constructor( node0, node1, voltage, resistance ) { super( node0, node1, null, 0 ); + + // @public this.voltage = voltage; + + // @public this.resistance = resistance; } } Index: js/model/ModifiedNodalAnalysisAdapter.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/model/ModifiedNodalAnalysisAdapter.js (revision c2fb8104fa1bee5d1ea013418527929336342db3) +++ js/model/ModifiedNodalAnalysisAdapter.js (date 1571372158348) @@ -12,6 +12,7 @@ const ACVoltage = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/model/ACVoltage' ); const Battery = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/model/Battery' ); const Capacitor = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/model/Capacitor' ); + const CCKCQueryParameters = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/CCKCQueryParameters' ); const circuitConstructionKitCommon = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/circuitConstructionKitCommon' ); const DynamicCircuit = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/model/DynamicCircuit' ); const Fuse = require( 'CIRCUIT_CONSTRUCTION_KIT_COMMON/model/Fuse' ); @@ -30,9 +31,10 @@ const timestepSubdivisions = new TimestepSubdivisions( errorThreshold, minDT ); class ResistiveBatteryAdapter extends DynamicCircuit.ResistiveBattery { - constructor( c, battery ) { super( c.vertexGroup.array.indexOf( battery.startVertexProperty.value ), c.vertexGroup.array.indexOf( battery.endVertexProperty.value ), battery.voltageProperty.value, battery.internalResistanceProperty.value ); + + // @public (read-only) this.battery = battery; } @@ -114,14 +116,14 @@ // TODO: Rename and documentation static apply( circuit, dt ) { - const batteries = []; // ResistiveBatteryAdapter - const resistors = []; // ResistorAdapter - const capacitors = []; // CapacitorAdapter - const inductors = []; // InductorAdapter + const resistiveBatteryAdapters = []; + const resistorAdapters = []; + const capacitorAdapters = []; + const inductorAdapters = []; for ( let i = 0; i < circuit.circuitElements.length; i++ ) { - const branch = circuit.circuitElements.get( i ); // Branch + const branch = circuit.circuitElements.get( i ); if ( branch instanceof Battery ) { - batteries.push( new ResistiveBatteryAdapter( circuit, branch ) ); + resistiveBatteryAdapters.push( new ResistiveBatteryAdapter( circuit, branch ) ); } else if ( branch instanceof Resistor || branch instanceof Fuse || @@ -131,34 +133,67 @@ // Since no closed circuit there; see below where current is zeroed out ( branch instanceof Switch && branch.closedProperty.value ) ) { - resistors.push( new ResistorAdapter( circuit, branch ) ); + resistorAdapters.push( new ResistorAdapter( circuit, branch ) ); } else if ( branch instanceof Switch && !branch.closedProperty.value ) { // no element for an open switch } else if ( branch instanceof Capacitor ) { - capacitors.push( new CapacitorAdapter( circuit, branch ) ); + capacitorAdapters.push( new CapacitorAdapter( circuit, branch ) ); } else if ( branch instanceof ACVoltage ) { - batteries.push( new ResistiveBatteryAdapter( circuit, branch ) ); + resistiveBatteryAdapters.push( new ResistiveBatteryAdapter( circuit, branch ) ); } else if ( branch instanceof Inductor ) { - inductors.push( new InductorAdapter( circuit, branch ) ); + inductorAdapters.push( new InductorAdapter( circuit, branch ) ); } else { assert && assert( false, 'Type not found: ' + branch.constructor.name ); } } - const dynamicCircuit = new DynamicCircuit( [], resistors, batteries, capacitors, inductors ); // new ObjectOrientedMNA() ); - const circuitResult = dynamicCircuit.solveWithSubdivisions( timestepSubdivisions, dt ); - batteries.forEach( batteryAdapter => batteryAdapter.applySolution( circuitResult ) ); - resistors.forEach( resistorAdapter => resistorAdapter.applySolution( circuitResult ) ); - capacitors.forEach( capacitorAdapter => capacitorAdapter.applySolution( circuitResult ) ); - inductors.forEach( inductorAdapter => inductorAdapter.applySolution( circuitResult ) ); + resistiveBatteryAdapters.forEach( batteryAdapter => batteryAdapter.battery.passProperty.reset() ); + resistiveBatteryAdapters.forEach( batteryAdapter => { + batteryAdapter.resistance = batteryAdapter.battery.internalResistanceProperty.value; + } ); + let dynamicCircuit = new DynamicCircuit( [], resistorAdapters, resistiveBatteryAdapters, capacitorAdapters, inductorAdapters ); + let circuitResult = dynamicCircuit.solveWithSubdivisions( timestepSubdivisions, dt ); + + // if any battery exceeds its current threshold, increase its resistance and run the solution again. + // see https://github.com/phetsims/circuit-construction-kit-common/issues/245 + let needsHelp = false; + resistiveBatteryAdapters.forEach( batteryAdapter => { + const current = circuitResult.getTimeAverageCurrent( batteryAdapter ); + if ( current > CCKCQueryParameters.batteryCurrentThreshold ) { + + batteryAdapter.battery.passProperty.value = 2; + batteryAdapter.resistance = batteryAdapter.battery.internalResistanceProperty.value; + console.log( 'set batteryAdapter resistance to ', batteryAdapter.battery.internalResistanceProperty.value ); + needsHelp = true; + } + else { + console.log( 'didnt need help on 1st pass' ); + } + } ); + if ( needsHelp ) { + dynamicCircuit = new DynamicCircuit( [], resistorAdapters, resistiveBatteryAdapters, capacitorAdapters, inductorAdapters ); + circuitResult = dynamicCircuit.solveWithSubdivisions( timestepSubdivisions, dt ); + } + + resistiveBatteryAdapters.forEach( batteryAdapter => { + const current = circuitResult.getTimeAverageCurrent( batteryAdapter ); + if ( batteryAdapter.battery.passProperty.value === 2 ) { + console.log( 'help arrived!', current ); + } + } ); + + resistiveBatteryAdapters.forEach( batteryAdapter => batteryAdapter.applySolution( circuitResult ) ); + resistorAdapters.forEach( resistorAdapter => resistorAdapter.applySolution( circuitResult ) ); + capacitorAdapters.forEach( capacitorAdapter => capacitorAdapter.applySolution( circuitResult ) ); + inductorAdapters.forEach( inductorAdapter => inductorAdapter.applySolution( circuitResult ) ); - //zero out currents on open branches + // zero out currents on open branches for ( let i = 0; i < circuit.circuitElements.length; i++ ) { const branch = circuit.circuitElements.get( i ); if ( branch instanceof Switch && !branch.closedProperty.value ) { Index: js/model/Battery.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/model/Battery.js (revision c2fb8104fa1bee5d1ea013418527929336342db3) +++ js/model/Battery.js (date 1571372226581) @@ -47,10 +47,15 @@ tandem: tandem.createTandem( 'voltageProperty' ) } ); + // @public + this.passProperty = new NumberProperty( 1 ); + // @public {Property.} the internal resistance of the battery - this.internalResistanceProperty = new DerivedProperty( [ internalResistanceProperty, this.currentProperty ], - ( internalResistance, current ) => { - if ( Math.abs( current ) >= CCKCQueryParameters.batteryCurrentThreshold ) { + this.internalResistanceProperty = new DerivedProperty( [ internalResistanceProperty, this.currentProperty, this.passProperty ], + ( internalResistance, current, pass ) => { + console.log( 'solving with pass = ' + pass ); + if ( pass === 2 ) { + // if ( Math.abs( current ) >= CCKCQueryParameters.batteryCurrentThreshold ) { return CCKCQueryParameters.batteryInternalResistanceWhenCurrentThresholdExceeded; } else { ```

[samreid]

I haven't enabled PhET-iO in package.json yet, so cannot publish a phet-io dev version, but here's a link to phettest that exercises the commits above (bear in mind that master is unstable and this may not be a good permanent reference)

shorted battery with separate light bulb circuit on phettest

I chose batteryInternalResistanceWhenCurrentThresholdExceeded=0.5 so that the animation wouldn't slow down too much. When I tried 0.1 it reduced the animation speed to 18% and the electrons were flowing very slowly through the light bulb.

[samreid]

At today's meeting, @ariel-phet and @arouinfar and I reviewed master with these values:

    batteryCurrentThreshold: {
      type: 'number',
      defaultValue: 1000
    },

    batteryInternalResistanceWhenCurrentThresholdExceeded: {
      type: 'number',
      defaultValue: 0.01
    }

We found the behavior (with the 2-phase matrix solve) is mostly what we need. @ariel-phet said he would test and fine-tune the behavior after I publish a dev version.

[samreid]

I published a dev version with query parameters for the battery threshold and the subsequent internal resistance. It uses the 2-phase strategy. I tested with these values:

https://phet-dev.colorado.edu/html/circuit-construction-kit-ac/1.0.0-dev.2/phet/circuit-construction-kit-ac_en_phet.html?batteryCurrentThreshold=10000&batteryInternalResistanceWhenCurrentThresholdExceeded=0.5

which means after 10,000 Amps, the battery will take a resistance of 0.5 Ohms. It had the desired behavior for this simple test case:

@ariel-phet can you please take a look?

[ariel-phet]

This solution is getting quite close, it works for most "normal" use cases, but with multiple batteries with high voltage, it gets a bit suspect (we start breaking ohm's law and such. @samreid said that we have easy access to the battery voltage and I think we can introduce a simple scaling factor to the internal resistance as a function of battery voltage (combined with the current threshold and 2 step solve already implemented).

@samreid once you have added this query parameter please reassign to me for testing/tweaking.

[samreid]

Here's a new version:

https://phet-dev.colorado.edu/html/circuit-construction-kit-ac/1.0.0-dev.4/phet/circuit-construction-kit-ac_en_phet.html

With these query parameters:

    batteryCurrentThreshold: {
      type: 'number',
      defaultValue: Number.POSITIVE_INFINITY
    },

    batteryInternalResistanceWhenCurrentThresholdExceededOffset: {
      type: 'number',
      defaultValue: 0
    },

    batteryInternalResistanceWhenCurrentThresholdExceededVoltageScaleFactor: {
      type: 'number',
      defaultValue: 0
    }

They are combined like so:

// @public {Property.<number>} the internal resistance of the battery
this.internalResistanceProperty = new DerivedProperty( [ this.voltageProperty, internalResistanceProperty, this.currentProperty, this.passProperty ],
  ( voltage, internalResistance, current, pass ) => {
    if ( pass === 2 ) {

      return CCKCQueryParameters.batteryInternalResistanceWhenCurrentThresholdExceededOffset +
             CCKCQueryParameters.batteryInternalResistanceWhenCurrentThresholdExceededVoltageScaleFactor * voltage;
    }
    else {
      return internalResistance;
    }
  } );

For example: &batteryCurrentThreshold=1000&batteryInternalResistanceWhenCurrentThresholdExceededVoltageScaleFactor=0.01

has current threshold 1000A, no offset and a scale factor of 0.01.

https://phet-dev.colorado.edu/html/circuit-construction-kit-ac/1.0.0-dev.4/phet/circuit-construction-kit-ac_en_phet.html?batteryCurrentThreshold=1000&batteryInternalResistanceWhenCurrentThresholdExceededVoltageScaleFactor=0.01

[ariel-phet]

Some parameters I found that worked well:

https://phet-dev.colorado.edu/html/circuit-construction-kit-ac/1.0.0-dev.4/phet/circuit-construction-kit-ac_en_phet.html?batteryCurrentThreshold=4000&batteryInternalResistanceWhenCurrentThresholdExceededOffset=4e-5&batteryInternalResistanceWhenCurrentThresholdExceededVoltageScaleFactor=2e-4

[samreid]

I turned the parameters from the preceding comment into the new defaults. @ariel-phet what's next for this issue?

[samreid]

CT discovered that the AC Voltage source can also exceed its current threshold, but it crashes since it doesn't have the extra features we added to Battery:

circuit-construction-kit-ac : fuzz : require.js : run
Query: brand=phet&ea&fuzz&memoryLimit=1000
Uncaught TypeError: Cannot set property 'value' of undefined
TypeError: Cannot set property 'value' of undefined
    at https://bayes.colorado.edu/continuous-testing/snapshot-1577512838163/circuit-construction-kit-common/js/model/ModifiedNodalAnalysisAdapter.js?bust=1577519272421:185:53
    at Array.forEach (<anonymous>)
    at Function.solveModifiedNodalAnalysis (https://bayes.colorado.edu/continuous-testing/snapshot-1577512838163/circuit-construction-kit-common/js/model/ModifiedNodalAnalysisAdapter.js?bust=1577519272421:183:32)
    at Circuit.step (https://bayes.colorado.edu/continuous-testing/snapshot-1577512838163/circuit-construction-kit-common/js/model/Circuit.js?bust=1577519272421:857:38)
    at ACVoltageModel.step (https://bayes.colorado.edu/continuous-testing/snapshot-1577512838163/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.js?bust=1577519272421:205:66)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1577512838163/joist/js/Sim.js?bust=1577519272421:228:22
    at Action.execute (https://bayes.colorado.edu/continuous-testing/snapshot-1577512838163/axon/js/Action.js?bust=1577519272421:230:20)
    at Sim.stepSimulation (https://bayes.colorado.edu/continuous-testing/snapshot-1577512838163/joist/js/Sim.js?bust=1577519272421:965:33)
    at Sim.stepOneFrame (https://bayes.colorado.edu/continuous-testing/snapshot-1577512838163/joist/js/Sim.js?bust=1577519272421:946:14)
    at Sim.runAnimationLoop (https://bayes.colorado.edu/continuous-testing/snapshot-1577512838163/joist/js/Sim.js?bust=1577519272421:929:14)
id: Bayes Chrome
Approximately 12/27/2019, 11:00:38 PM

Should the "if any battery exceeds its current threshold, increase its resistance and run the solution again" rule apply to AC Sources as well? Or should I opt-out for them?

[ariel-phet]

For design meeting we should look at the behavior of the sim with the new parameters I suggested in https://github.com/phetsims/circuit-construction-kit-common/issues/245#issuecomment-568650850 (and that @samreid has now made defaults). A few things to consider:

1. For a single battery and single bulb with a short, these parameters work well over the entire voltage range

2. For multiple batteries (eg 5 connected in series), a single bulb, and a short, these parameters also work well, although since the wires are stretched further a tiny glow can be seen if the wires are stretched more.

3. We could consider tweaking the resistance of wires (I believe @samreid and I looked at this mapping and it seemed a bit off when considering the length of wires and assuming something near 12 gauge copper wire). Basically pixel to wire length conversion seemed to be making wires fairly long. This might be a consideration for a separate issue

4. We could also consider tweaking the mapping of "light bulb" glow to current. I think if we made the bulb start not to glow until above even 0.15 amps we would be good in all but the most extreme cases. We might also consider what is realistic here...at what amperage does a normal incandescent bulb actually glow?

5. As to @samreid question about AC voltage sources, I just tried shorting one, and the behavior looks super wrong to me, so we should visit that issue as well.

[samreid]

From today's meeting, we would like to try apply the same pattern from batteries to AC voltage sources.

[arouinfar]

1/9/20 design meeting notes:

• The values recommended by @ariel-phet look good.
• We decided to proceed with adjusting the length scaling of the wires, see #553
• We did not make a decision about (4) in https://github.com/phetsims/circuit-construction-kit-common/issues/245#issuecomment-571666663
• @samreid will try to generalize the pattern to the AC voltage sources

@ariel-phet I don't know that we need to make a threshold for showing the light rays. I realize that there is likely such a threshold in reality, but I like the simplicity of linearly scaling the glow until the max brightness is reached at 2000 W. I do not believe we've had any complaints about this behavior.

[samreid]

For reference, here is the brightness function which is called with a multiplier of 0.35:

  /**
   * Determine the brightness for a given power
   * @param {number} multiplier - steepness of the function
   * @param {number} power - the power through the light bulb
   * @returns {number}
   */
  const toBrightness = ( multiplier, power ) => {
    const maximumBrightness = 1;

    // power at which the brightness becomes 1
    const maximumPower = 2000;
    return Math.log( 1 + power * multiplier ) * maximumBrightness / Math.log( 1 + maximumPower * multiplier );
  };

[samreid]

In the commits, I generalized the "short circuiting voltage source" functionality to apply to both Battery and ACVoltage, and I identified and resolved two sign errors. It's behaving much better now. @ariel-phet can you please review on phettest? If that checks out--is there anything else to do for this issue?

[ariel-phet]

I think with the latest dev https://phet-dev.colorado.edu/html/circuit-construction-kit-ac/1.0.0-dev.5/phet/circuit-construction-kit-ac_en_phet.html and the various fixes we have made to wire resistance and such, all is looking good here. Closing

[samreid]

Reopening based on the changes in https://github.com/phetsims/circuit-construction-kit-common/issues/676. We would like to try reverting the changes in this issue to see if it is already covered by https://github.com/phetsims/circuit-construction-kit-common/issues/676.

[samreid]

I removed the code that changes the battery internal resistance when a current is exceeded, and observed that the light bulb still lit up in this case:

However, when I specified: wireResistivity=1E-11&batteryMinimumResistance=1E-6 then it was correct:

Since we were leaning towards these lower values, I'll go ahead and commit, but we are considering changing the values further in https://github.com/phetsims/circuit-construction-kit-common/issues/676 and we'll need to make sure this case is still handled by the new values.

[samreid]

Committed, @arouinfar can you please test this in a few cases?

[arouinfar]

@samreid I tested several circuits in https://github.com/phetsims/circuit-construction-kit-common/issues/676#issuecomment-810727725. The compromise is that we won't be able to completely turn off the bulb in all situations without turning the internal resistance up high enough to see numerical artifacts, but for most reasonable circuits the bulb will go out.

[samreid]

It sounds promising, but I wanted to clarify if this issue can be closed (i.e. without a battery current threshold that increases its resistance).