assertion failure: percentConcentration out of range

[pixelzoom]

Steps to reproduce, from #209 (comment):

1. master BLL, Concentration screen

2. Drain the beaker.
3. Shake particles, and keep shaking until some hit the bottom of the beaker.
4. Quickly, while some particles are still falling (and some are on the bottom too), start adding water to the beaker.
5. Drain the beaker and repeat if no errors were thrown.

Assertion thrown:

Uncaught Error: Assertion failed
    at window.assertions.assertFunction (/assert/js/assert.js:22)
    at ConcentrationSolution.js?bust=1507652942716:112
    at Array.listener (DerivedProperty.js?bust=1507652942716:67)
    at Emitter.emit2 (Emitter.js?bust=1507652942716:205)
    at DerivedProperty._notifyListeners (Property.js?bust=1507652942716:207)
    at DerivedProperty._setAndNotifyListeners (Property.js?bust=1507652942716:195)
    at DerivedProperty.set (Property.js?bust=1507652942716:142)
    at Array.listener (DerivedProperty.js?bust=1507652942716:67)
    at Emitter.emit2 (Emitter.js?bust=1507652942716:205)
    at DerivedProperty._notifyListeners (Property.js?bust=1507652942716:207)

ConcentrationSolution relevant code:

        var percentConcentration = 0;
        if ( volume > 0 ) {
          var solventGrams = volume * self.solvent.density;
          percentConcentration = 100 * ( soluteGrams / ( soluteGrams + solventGrams ) );
        }
        assert && assert( percentConcentration >= 0 && percentConcentration <= 100 );

[pixelzoom]

Looks like the problem is with soluteGramsProperty. It's sometimes negative, should be >= 0, and I've added an assertion for that (see above commit).

The steps to reproduce can now be truncated to:

1. master BLL, Concentration screen
2. Drain the beaker.
3. Shake particles, and keep shaking until some hit the bottom of the beaker.

As soon as a particle hits the bottom of the beaker, you'll see an assertion failure with message like this:

Assertion failed: invalid soluteGrams: -1.71148

[pixelzoom]

ConcentrationSolution defines soluteGramsProperty like this:

this.soluteGramsProperty = new DerivedProperty( 
  [ this.soluteProperty, this.soluteAmountProperty, this.precipitateAmountProperty ],
  function( solute, soluteAmount, precipitateAmount ) {
    var soluteGrams = solute.molarMass * ( soluteAmount - precipitateAmount );
    assert && assert( soluteGrams >= 0, 'invalid soluteGrams: ' + soluteGrams );
    return soluteGrams;
  }

What I'm seeing is that the value of the soluteAmount arg does not match the value of the soluteAmountProperty dependency. If I add this assertion to the DerivedProperty's function, if fails:

assert && assert( soluteAmount === self.soluteAmountProperty.get(),
  'soluteAmount=' + soluteAmount + 
  ', soluteAmountProperty.get()=' + self.soluteAmountProperty.get() );

with this error message:

Assertion failed: soluteAmount=0, soluteAmountProperty.get()=0.005

I do not understand what's going on here. Could this be happening because precipitateAmountProperty is also a DerivedProperty with soluteAmountProperty as a dependency?

[pixelzoom]

I asked:

Could this be happening because precipitateAmountProperty is also a DerivedProperty with soluteAmountProperty as a dependency?

Hmm, could be. If I remove this.precipitateAmountProperty as a dependency of soluteGramsProperty, then the value of the soluteAmount matches the value of the soluteAmountProperty dependency.

[pixelzoom]

Here's a simple example that's similar to what I have in ConcentrationSolution:

var aProperty = new NumberProperty( 0 );
var bProperty = new DerivedProperty( [ aProperty ], 
  function( a ) { return a + 1; } );
var cProperty = new DerivedProperty( [ aProperty, bProperty ],
  function( a, b ) {
    assert && assert( a === aProperty.get(), 'a=' + a + ', aProperty.get()=' + aProperty.get() );
    return a + b;
} );

Calling aProperty.set(1) fails the assertion with this message:

Assertion failed: a=0, aProperty.get()=1

If I replace bProperty with var bProperty = new NumberProperty( 0 );, there's no problem.

This sure looks like a general problem to me. @samreid @jonathanolson can you have a look at the example above? Would you expect it to fail? If so, why?

[pixelzoom]

Ah... The problem is in this bit of DerivedProperty:

47 // @private Keep track of each dependency and only update the changed value, for speed
48 this.dependencyValues = dependencies.map( function( property ) {return property.get();} );

In the above example, when bProperty changes, cProperty's derivation function will be called with the new value of bProperty and the old value of aProperty.

[pixelzoom]

Here's the above example again, with a console message instead of an assertion:

var aProperty = new NumberProperty( 0 );
var bProperty = new DerivedProperty( [ aProperty ], 
  function( a ) { return a + 1; } );
var cProperty = new DerivedProperty( [ aProperty, bProperty ],
  function( a, b ) {
    console.log( 'a=' + a + ' aProperty.get()=' + aProperty.get() );
    return a + b;
} );

Console output demonstrating that cProperty's derivation function is first called with the old value of aProperty, then with the new value of aProperty:

> aProperty.set( 1 )
a=0 aProperty.get()=1
a=1 aProperty.get()=1
> aProperty.set( 2 )
a=1 aProperty.get()=2
a=2 aProperty.get()=2

[pixelzoom]

This is a serious problem in DerivedProperty. It's dependencies can be any array of Property - and that includes DerivedProperty, since it's a subtype of Property. And in some scenarios (like the one above) it's most definitely not calling the derivation function with the current values of all dependencies.

[pixelzoom]

I created an axon issue for the DerivedProperty problem, see phetsims/axon#146.

[pixelzoom]

This issues requires phetsims/axon#146 to be addressed. Blocked until then.

[pixelzoom]

This was resolved by phetsims/scenery#700 (comment), nothing sim-specific needs to be changed.

@phet-steele please verify in master.

[phet-steele]

This was resolved by phetsims/scenery#700 (comment), nothing sim-specific needs to be changed.

@phet-steele please verify in master.

@pixelzoom I think you meant phetsims/axon/issues/146? Anyway, this is fixed in master.

[pixelzoom]

@phet-steele You think correctly. Thanks for verifying.