search

phetsims / beers-law-lab

"Beer's Law Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/beers-law-lab
GNU General Public License v3.0
2 stars 9 forks source link

CT: should have type: ShakerParticleIO in allTypes #244

Closed samreid closed 4 years ago

samreid commented 4 years ago

On Slack, @pixelzoom pointed out:

beers-law-lab and concentration are failing CT with "should have type: ShakerParticleIO in allTypes. May need grunt update?" But running grunt update results in no changes.

This problem can be reproduced by running Beer's Law Lab in studio, then shaking out some solute from the shaker.

I suspect the problem is that Beer's Law Lab is using the legacy pattern for dynamic state, and hence ShakerParticleIO is not created during baseline creation. Porting to the new PhetioGroup pattern should address this.

This longstanding problem was recently revealed when studio started running on CT again recently.

pixelzoom commented 4 years ago

Also occurs in Concentration.

The stack trace is:

beers-law-lab : phet-io-studio-fuzz : require.js : run
Uncaught Error: should have type: ShakerParticleIO in allTypes. May need grunt update?
Error: should have type: ShakerParticleIO in allTypes. May need grunt update?
    at window.assert (https://bayes.colorado.edu/continuous-testing/snapshot-1582032248597/phet-io-wrappers/common/js/assert.js:32:13)
    at getCreator (https://bayes.colorado.edu/continuous-testing/snapshot-1582032248597/studio/js/StudioInstanceCreator.js:778:15)
    at StudioInstanceCreator.createStudioInstance (https://bayes.colorado.edu/continuous-testing/snapshot-1582032248597/studio/js/StudioInstanceCreator.js:61:26)
    at Studio.onPhetioElementAdded (https://bayes.colorado.edu/continuous-testing/snapshot-1582032248597/studio/js/Studio.js:276:63)
    at phetioElementAdded (https://bayes.colorado.edu/continuous-testing/snapshot-1582032248597/studio/js/main.js:50:26)
    at Client.dispatch (https://bayes.colorado.edu/continuous-testing/snapshot-1582032248597/phet-io-wrappers/common/js/Client.js:452:56)
    at windowMessageListener (https://bayes.colorado.edu/continuous-testing/snapshot-1582032248597/phet-io-wrappers/common/js/Client.js:341:24)
id: Bayes Chrome
Approximately 2/18/2020, 6:24:08 AM
pixelzoom commented 4 years ago

I'm guesing that we'll need to do the same for SoluteParticle, which is a subclass of ShakerParticle.

pixelzoom commented 4 years ago

Slack phet-io channel:

Chris Malley 1:48 PM Can someone from the PhET-iO team point me to an example of "the new PhetioGroup pattern" so that I can address https://github.com/phetsims/beers-law-lab/issues/244?

Sam Reid 1:50 PM If I recall correctly, “Charges and Fields” uses this pattern properly. However, I wonder if it would be best to schedule a collaboration with CK, SR or MK as you take the first steps on this. I’m available until 2:30pm if you want to team up

Michael Kauzmann 1:53 PM ProjectileMotionModel.trajectories could also be helpful. I have to focus on the es6 module sprint this week, but could do something next week. Also note that the projectile motion pattern factors out the group creation to Trajectory.createGroup , but that is not a convention, and instead more of a "tool if desired"

Chris Malley 1:57 PM I'll see if I can make progress on my own. If not, I'll defer.

pixelzoom commented 4 years ago

I made baby steps, then bailed. Here's a patch (without grunt update):

Patch ```diff Index: js/concentration/model/ShakerParticle.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/concentration/model/ShakerParticle.js (revision 09151a539ae235756a7ac151bae396950dc4f7f9) +++ js/concentration/model/ShakerParticle.js (date 1582667899436) @@ -15,8 +15,12 @@ const beersLawLab = require( 'BEERS_LAW_LAB/beersLawLab' ); const inherit = require( 'PHET_CORE/inherit' ); const merge = require( 'PHET_CORE/merge' ); + const PhetioGroup = require( 'TANDEM/PhetioGroup' ); + const PhetioGroupIO = require( 'TANDEM/PhetioGroupIO' ); const ShakerParticleIO = require( 'BEERS_LAW_LAB/concentration/model/ShakerParticleIO' ); + const Solute = require( 'BEERS_LAW_LAB/concentration/model/Solute' ); const SoluteParticle = require( 'BEERS_LAW_LAB/concentration/model/SoluteParticle' ); + const Vector2 = require( 'DOT/Vector2' ); /** * @param {Solute} solute @@ -29,10 +33,6 @@ */ function ShakerParticle( solute, position, orientation, initialVelocity, acceleration, options ) { - options = merge( { - phetioType: ShakerParticleIO - }, options ); - SoluteParticle.call( this, solute.particleColor, solute.particleSize, position, orientation, options ); // @public (read-only, phet-io) @@ -43,6 +43,33 @@ beersLawLab.register( 'ShakerParticle', ShakerParticle ); + /** + * Creates a PhetioGroup for ShakerParticles, which are dynamically created. + * @param {Tandem} tandem + */ + ShakerParticle.createGroup = tandem => { + return new PhetioGroup( + + // createMember + ( tandem, solute, position, orientation, initialVelocity, acceleration ) => { + return new ShakerParticle( solute, position, orientation, initialVelocity, acceleration, { + tandem: tandem, + phetioType: ShakerParticleIO + } ); + }, + + // defaultArguments + [ Solute.DRINK_MIX, Vector2.ZERO, 0, Vector2.ZERO, Vector2.ZERO ], + + // options + { + tandem: tandem, + phetioType: PhetioGroupIO( ShakerParticleIO ), + phetioDocumentation: 'The container for shaker particles that are dynamically created' + } + ); + }; + return inherit( SoluteParticle, ShakerParticle, { /** Index: js/concentration/model/ShakerParticles.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/concentration/model/ShakerParticles.js (revision 09151a539ae235756a7ac151bae396950dc4f7f9) +++ js/concentration/model/ShakerParticles.js (date 1582667305212) @@ -39,6 +39,8 @@ function ShakerParticles( shaker, solution, beaker, options ) { options = merge( { + + // phet-io tandem: Tandem.REQUIRED, phetioType: ShakerParticlesIO }, options ); @@ -58,7 +60,7 @@ } ); // @private - this.shakerParticleGroupTandem = options.tandem.createGroupTandem( 'shakerParticle' ); + this.shakerParticlesGroup = ShakerParticle.createGroup( options.tandem.createTandem( 'shakerParticlesGroup' ) ); } beersLawLab.register( 'ShakerParticles', ShakerParticles ); @@ -113,14 +115,12 @@ if ( shaker.dispensingRateProperty.get() > 0 ) { const numberOfParticles = Utils.roundSymmetric( Math.max( 1, shaker.dispensingRateProperty.get() * solution.soluteProperty.get().particlesPerMole * deltaSeconds ) ); for ( let j = 0; j < numberOfParticles; j++ ) { - const shakerParticle = new ShakerParticle( + const shakerParticle = this.shakerParticlesGroup.createNextMember( solution.soluteProperty.get(), getRandomPosition( this.shaker.positionProperty.get() ), getRandomOrientation(), this.getInitialVelocity(), - this.getGravitationalAcceleration(), { - tandem: this.shakerParticleGroupTandem.createNextTandem() - } + this.getGravitationalAcceleration() ); this.addParticle( shakerParticle ); } ```

In ShakerParticle.js, I added:

  /**
   * Creates a PhetioGroup for ShakerParticles, which are dynamically created.
   * @param {Tandem} tandem
   */
  ShakerParticle.createGroup = tandem => {
    return new PhetioGroup(

      // createMember
      ( tandem, solute, position, orientation, initialVelocity, acceleration ) => {
        return new ShakerParticle( solute, position, orientation, initialVelocity, acceleration, {
          tandem: tandem,
          phetioType: ShakerParticleIO
        } );
      },

      // defaultArguments, passed to createMember during API harvest
      [ Solute.DRINK_MIX, Vector2.ZERO, 0, Vector2.ZERO, Vector2.ZERO ],

      // options
      {
        tandem: tandem,
        phetioType: PhetioGroupIO( ShakerParticleIO ),
        phetioDocumentation: 'The container for shaker particles that are dynamically created'
      }
    );
  };

In ShakerParticles.js (where ShakerParticle instances are created) I created the PhetioGroup in the constructor:

    // @private
    this.shakerParticlesGroup = ShakerParticle.createGroup( options.tandem.createTandem( 'shakerParticlesGroup' ) );

Then created ShakerParticle like this in step:

          const shakerParticle = this.shakerParticlesGroup.createNextMember(
            solution.soluteProperty.get(),
            getRandomPosition( this.shaker.positionProperty.get() ),
            getRandomOrientation(),
            this.getInitialVelocity(),
            this.getGravitationalAcceleration()
          );

When running in Studio with http://localhost/~cmalley/GitHub/studio/?sim=beers-law-lab&phetioValidateTandems&phetioDebug&phetioElementsDisplay=all, shaking the shaker results in this Error:

Uncaught Error: Assertion failed: PhET-iO API error:
beersLawLab.concentrationScreen.model.shakerParticles.shakerParticlesGroup.shakerParticles_0:  4. After startup, only dynamic instances prescribed by the baseline file can be registered.
    at window.assertions.assertFunction (assert.js?bust=1582668443036:22)
    at PhetioAPIValidation.assertAPIError (phetioAPIValidation.js?bust=1582668443127:280)
    at phetioAPIValidation.js?bust=1582668443127:211
    at callback (timer.js?bust=1582668443127:42)
    at TinyEmitter.emit (TinyEmitter.js?bust=1582668443127:68)
    at Emitter.js?bust=1582668443127:36
    at Timer.execute (Action.js?bust=1582668443127:227)
    at Timer.emit (Emitter.js?bust=1582668443127:61)
    at Sim.js?bust=1582668443127:271
    at Action.execute (Action.js?bust=1582668443127:227)
pixelzoom commented 4 years ago

After converting beers-law-lab to ES6 classes, this Error has morphed into this:

beers-law-lab : phet-io-fuzz : require.js : load
Query: brand=phet-io&phetioStandalone&ea&phetioValidateTandems&fuzz&memoryLimit=1000
Uncaught TypeError: Right-hand side of 'instanceof' is not an object
TypeError: Right-hand side of 'instanceof' is not an object
    at Object.isValidValue (https://bayes.colorado.edu/continuous-testing/snapshot-1582780752950/beers-law-lab/js/concentration/model/SoluteIO.js?bust=1582782417553:65:47)
    at Object.isValueValid (https://bayes.colorado.edu/continuous-testing/snapshot-1582780752950/axon/js/ValidatorDef.js?bust=1582782417553:300:69)
    at validate (https://bayes.colorado.edu/continuous-testing/snapshot-1582780752950/axon/js/validate.js?bust=1582782417553:30:20)
    at new ObjectIO (https://bayes.colorado.edu/continuous-testing/snapshot-1582780752950/tandem/js/types/ObjectIO.js?bust=1582782417553:41:7)
    at new SoluteIO (https://bayes.colorado.edu/continuous-testing/snapshot-1582780752950/beers-law-lab/js/concentration/model/SoluteIO.js?bust=1582782417553:19:3)
    at Solute.initializePhetioObject (https://bayes.colorado.edu/continuous-testing/snapshot-1582780752950/tandem/js/PhetioObject.js?bust=1582782417553:352:30)
    at new PhetioObject (https://bayes.colorado.edu/continuous-testing/snapshot-1582780752950/tandem/js/PhetioObject.js?bust=1582782417553:192:12)
    at new Solute (https://bayes.colorado.edu/continuous-testing/snapshot-1582780752950/beers-law-lab/js/concentration/model/Solute.js?bust=1582782417553:54:7)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1582780752950/beers-law-lab/js/concentration/model/Solute.js?bust=1582782417553:89:22
    at Object.execCb (https://bayes.colorado.edu/continuous-testing/snapshot-1582780752950/sherpa/lib/require-2.3.6.js?bust=1582782417442:1696:33)
id: Bayes Chrome
Approximately 2/26/2020, 10:19:12 PM

Here's the line that's failing in SoluteIO. phet.beersLawLab.Solute is apparently "not an object".

SoluteIO.validator = { isValidValue: v => v instanceof phet.beersLawLab.Solute };
pixelzoom commented 4 years ago

@samreid and I paired on conversions of ShakerParticle via Zoom. Then I worked on PrecipitateParticle, followed by generalization and cleanup. See above commits. Tested with standalone, state wrapper, and Studio.

@samreid would you please review, since you're most familiar with this?

Finally, some thoughts on this task, for the benefit of @samreid, @zepumph, @chrisklus, @kathy-phet, and @ariel-phet:

pixelzoom commented 4 years ago

See also a potential performance impact of using PhetioGroup, described in https://github.com/phetsims/beers-law-lab/issues/209#issuecomment-593625698.

samreid commented 4 years ago

Generally, this usage looks good, and it has nice behavior in the state wrapper. Some review comments and questions:

pixelzoom commented 4 years ago
  • It looks like ShakerParticles.createGroup should be removed.

Good catch, thanks, done in above commits.

  • PrecipitateNode.js isn't adding too much, perhaps it should be deleted. I don't feel too strongly about this one.

I prefer to keep it, for symmetry with ShakerParticlesNode. And it's unpacking ShakerParticles to super constructor args.

  • Will you also need to grunt update in concentration?

Done in above commits.

  • When shaking solute out in studio, and pressing the "launch" button, an extra spurious piece of solute appears after launching. May be a pre-existing problem and/or tracked in another issue?

No idea, but I can't spend any more time on this now. Tracking in #247.

Closing.

zepumph commented 4 years ago

Contrary to previous discussions about PhetioGroup, it was not at all easy to add a PhetioGroup and keep the same data structures for managing particles. We ended up removing the original data structures and using the PhetioGroup as the data structure. I suspect that this will be true in general, which is unfortunate because it generally means significant code changes.

I am just entering this discussion now, so forgive me if we have discussed this, but did you thing about creating a separate PhetioGroup in the model and then creating an ObservableArray that listens to the PhetioGroup and populates itself based soley on the PhetioGroup? This is perhaps a bit redundant, but could potentially lead to less refactoring of code that had been interfacing with an array up until this github issue's work.

I mention this just as a potential "tool in our phet-io instrumentation" toolbox, though the phet-io team has not yet exercised this pattern because we have found that converting usage sites of the array to work with the PhetioGroup was not too bad.

Only reopen if you would like to continue discussion.

pixelzoom commented 4 years ago

After this work was completed, @kathy-phet noted that we probably didn't need to instrument individual particles. Oh well, at least I learned something about PhetioGroup.

samreid commented 4 years ago

@kathy-phet, @chrisklus and I discussed PhetioGroup usage earlier this week and, in my opinion, this usage of PhetioGroup is preferable to creating a function that re-randomizes the positions/locations of shaker particles or precipitate particles based on their number. When we design the phet-io API for this sim, we may or may not decide that the falling solute particles are "transient" and shouldn't be instrumented at all, but I don't think we would decide that for the precipitate particles.