PhET-iO instrumentation

[pixelzoom]

At 8/16/18 dev meeting, @ariel-phet said that we should attempt to do PhET-iO instrumentation as part of initial instrumentation for this sim. So we need to specify the PhET-iO API -- what needs to be instrumented, tandem name, etc.

Setting priority of this to "medium". It would be nice to have this specification completed soon, so that instrumentation occurs while the code is being written, rather than as a separate (later) task.

---

UPDATE from @samreid: I'm commandeering this comment for the PhET-iO Instrumentation checklist. For this issue, a checkmark indicates it has been reviewed. Discovered problems will be noted as new issues or comments in this issue.

How to Instrument a PhET Simulation for PhET-iO

Before instrumenting

• Create a "PhET-iO Instrumentation" issue in the simulation repository. Copy this checklist to the issue description (top issue comment) for tracking. Link back to this checklist via /blob/<SHA>/ so that the specific guide you used is preserved. https://github.com/phetsims/phet-io/blob/a7436d70e1671c0a12e007356b9a4c8d42e6e5b3/doc/how-to-instrument-a-phet-simulation-for-phet-io.md
• Understand the goal. Launch the Faraday's Law wrapper index, read through it and visit all of the linked wrappers and docs. Test each wrapper, investigate, report bugs, ask questions!
• Create a dev release before instrumentation. This creates a benchmark to reference against for memory-leaks, sim size, performance, etc. Document the dev release in the sim's phet-io github issue.
• Schedule a PhET-iO design meeting for the simulation to identify what should be customizable/interoperable/data stream and track it in an issue. For example, see how-to-design-phet-io-features-for-a-simulation.md Think about how a researcher or 3rd party may wish to configure the simulation or collect data from it, and make sure that is supported by the instrumentation. For example, some simulations will need custom higher-level events (such as whether the user created a parallel circuit), for events that are useful, easy to compute in simulation code and difficult to compute in wrapper code. Or a simulation may need to be configurable in a way that is not already supported by the instrumentation you have already completed. These features should be determined in the PhET-iO design meeting. Noted in PhET-iO instrumentation #14 (comment)
• Typically it is best if the responsible developer for the simulation is available to perform the PhET-iO instrumentation. They have important insight into the structure, history, trade-offs and other important details of the simulation implementation that will facilitate the instrumentation. If the responsible developer is not available for instrumentation, it would be nice if they are available for consultation or support during instrumentation.

Code Review

A high-quality Code Review will make instrumentation easier, promote long term maintainability for the
simulation, and protect the simulation from a volatile API. If the simulation is already in good shape, the review
will not take too long. If the simulation is not in good shape, then it needs your help.

• Read through the open issues and be aware of any outstanding problems, future work, etc.
• If there is a branch with significant effort, consider merging it before instrumentation.
• Complete any planned refactorings.
• Address TODOs in the code
• Bring the sim up to standards.
• If there are sim components that can be exchanged to use newer common code ones, do so. Consulting phet design patterns may be helpful.

Instrumentation

Now that the simulation is in good shape and the PhET-iO design meeting is complete, we are ready to instrument the simulation.
Follow the checklist below, and if you have questions you can review Faraday's Law and its PhET-iO instrumentation or
reach out to teammates who may have come this way before.

Initial Setup

• Add 'phet-io' as a supportedBrand in the sim's package.json. A script on Bayes will automatically add the simulation to the list of phet-io simulations. This will make it possible to use phetmarks to launch wrappers for testing. But be warned, this also will add it to continuous fuzz testing, so only commit that if you are ready for Bayes CT to begin testing the PhET-iO instrumentation.
• Import Tandem to main.js, see faradays-law-main.js for an example
• Pass tandems to each screen using tandem.createTandem(...)

Visit Objects that Should be Instrumented

Consult the PhET-iO design issue to see what features the sim should support. See
https://github.com/phetsims/tandem/tree/master/js/PhetioObject.js for the
supported PhetioObject options. Not every node in the hierarchy must be instrumented, but every leaf is instrumented.
For example the view is rarely instrumented. Use phetioValidateTandems=false to test the links from the wrapper
index with a partially-instrumented simulation.

• Recursively pass tandems and other PhetioObject options into objects that should be instrumented. Do not instrument objects that are "implementation details" and do not over-instrument.
• Instrument user interface components such as Checkbox, HSlider, etc.
• Instrument model components such as Axon Property that are critical to the save state or operation of the sim. This does not necessarily include "implementation details" that should be hidden from the public API; again a design meeting may be needed here. (Note that some Property sub-classes utilize options specific for use with PhET-IO, units in NumberProperty for example, and should be passed where appropriate.)
• Instrument all of the features identified in the simulation PhET-iO design issue, see PhET-iO instrumentation #14 (comment)

Creating and Naming Tandems

Well-designed tandem names are important. Once the PhET-iO simulation is published, the API becomes public and therefore
difficult to change. Sometimes PhET-iO design meetings can also help come up tandem names. NOTE: "Tandem" is a PhET
internal name, publicly to clients the full strings are known as "phetioIDs."

• Screen instance tandems should end with a Screen suffix.
• Property instances should have Property suffix.
• The screen's model and view should be named model and view.
• Tandems should be named as we wish clients to see them, and for long-term stability. For maintainability, local vars should be renamed to match tandem names.
• When adding tandem args to constructors, please follow the following heuristic regarding required vs optional tandem args from Should tandem be in options or required param? joist#489 (comment):
  • Use @param tandem for constructors that don't have an options parameter. This typically includes top-level model and view types that are specific to the sim.
  • Use tandem in options object: Tandem.required for constructors that already have an options parameter. This default can be helpful for identifying cases where you have neglected to pass a tandem in (because Tandem.required will error loudly if validating tandems).
• Use createGroupTandem for arrays or otherwise numbered tandems. See usages for examples.
• Use static tandems where necessary. For instance, static objects that are not created from the main sequence.
  See BeersLawSolution.js for an example.

Feature Support

• Where appropriate, create or instrument Property instances to make it possible to get/set a value, so value changes will appear on the data stream and so the item can be stored and restored in save/load.
• If necessary, instrument common code components that are not yet instrumented. You can check if something is instrumented by checking whether it extends PhetioObject and whether it supplies any PhetioObject options. To instrument a new common code component, you may need to add instrumented Property or Emitter elements by composition, or subclass PhetioObject.
• Add tandem: Tandem.required or tandem: Tandem.optional to the options accordingly
• Note Node already extends PhetioObject--its PhetioObject options should be provided to the constructor or mutate but not both.
• Use the printMissingTandems flag if you want to collect a list of all required, optional, and uninstrumented common code classes instead of erroring out on the first missing tandem. Each occurrence is numbered to give a better idea of how many the sim has to do.
• Transient State should not be saved. For instance, whether a button is highlighted from mouseover, or whether the About dialog is showing should not be part of the save state of the simulation.
• Run the simulation with ?phetioValidateTandems=true to see if you missed anything that should be instrumented.

Create new IO types

If necessary, create new IO types to support desired feature set. Generally we don't want to be locked in to coupling
IO Types to sim types. Instead, we decided that we want the PhET-iO API to be able to vary independently from the sim
implementation instead of leaking sim implementation details. Still, for a well-designed simulation, IO Types will
often match closely with the sim types. To ensure good IO type inheritance hierarchies follow these principles:

• factor out duplicated code or responsibilities
• having the sim developer involved in instrumentation
• making sure everything is reviewed
  See sloppy TTypes beers-law-lab#213 for more context on prior problems in this area and discussion
  about it.

The Data Stream

• Create Emitter instances as appropriate to augment the data stream.
• Instrumented Emitters and Property instances naturally emit to a structured data stream and are probably what you need. If you need something more custom, you can call phetioStartEvent and phetioEndEvent directly.
• Disabled components should not deliver events, even when clicked. Change them to be pickable=false when disabled. See https://github.com/phetsims/phet-io/issues/282
• New code should use Emitter.addListener instead of Events.onStatic
• To suppress an Emitter.emitN argument, you may specify VoidIO for its type, see PressListener.js
• Make sure that events are marked as phetioEventType: 'user' for pointer events, keyboard events and UI events (like checkbox toggled, button pressed), and phetioEventType: 'model' for model actions/responses. This is easiest to test in the console: colorized wrapper. Model events will be logged in black, and user events will be logged blue. You can also go to the data-stream wrapper to see events in JSON form.

Post Instrumentation and Checks

• Make sure unused PhetioObject instances are disposed, which unregisters the tandem.
• Make sure Joist dt values are used instead of Date.now() or other Date functions. This is necessary for reproducible playback via input events. Perhaps try phet.joist.elapsedTime.
• Are random numbers using phet.joist.random, and all doing so after modules are declared (non-statically)? For example, the following methods (and perhaps others) should not be used: Math.random, _.shuffle, _.sample, _.random.
• undefined values cannot be saved by phet-io, sims should be written to use null instead.
• Verify that the simulation works in all of the phet-io wrappers.
• Build with grunt --brands=phet-io and test the built version by launching build/wrappers/index and testing all the links.
• Manually look through Studio to make sure that tandems work as expected and are formatted correctly.
• Perform a full test for memory leaks. The benchmark dev release can be helpful here. This will help catch faulty tandem disposal. PhET-iO instantiates different objects and wires up listeners that are not present in the PhET-branded simulation. It needs to be tested separately for memory leaks.
• Run phetmarks=>aqua=>Test Sims(Fast Build) with PhET-iO checked. This will help catch any simulations using the component you just instrumented. Next you will need to pass tandems for those cases.

Support dynamic state

For simulations that have static content (such as a fixed number of objects and properties), instrumentation
is complete after you have completed the preceding steps. For simulations that have a dynamic number of objects,
such as Circuit Construction Kit circuits or Molecules and Light photons, the containers and elements must be instrumented.
This is currently tricky with PhET-iO. Some sims may wish to avoid this entire hassle by pre-allocating all of the
instrumented instances. Consider adding flags to indicate whether the objects are "alive" or "in the pool".

Details about how to support dynamic state.

Beer's Law Lab and Charges and Fields demonstrates how this may be done. A container class defines two methods:
clearChildInstances which empties a container and addChildInstance which repopulates a container one element at a
time. For example, see ShakerParticlesIO in the beers-law-lab instrumentation.

When state is set, first the container is cleared, then children are created. Child states can be obtained from toStateObject
and set back with fromStateObject, with an additional call to setValue in case additional data is supplied, or custom
code can be used.

Dispose must be implemented properly on all dynamic instances, or else it will result in stale values in the playback sim.
For example, if a simulation is sending the position of a particle as a property, if the particle position property
hasn't been disposed of, the simulation will try to create a new property with the same id and hence throw an assertion
error because that tandem is already registered.

On January 11, 2017 ControlPoints were not being disposed correctly in Energy-skate-park-basics, causing a mysterious bug
(impossible set state), make sure that children are being disposed correctly before creating them in the downstream sim!

Other tips and tricks for "impossible set state":

• addChildInstance must return the instance, it is used as a flag to determine whether addition was successful
• the given tandems must be reused. Do not use GroupTandem to assign a new tandem, use the specified tandem so the object
  can be addressed the same way

Dispose functions must be added to types that are instrumented. But that's only half of the memory management issue. The
other half is revisiting memory management for all instances that don't exist for the lifetime of the sim, and verifying
that tandems are properly cleaned up.

Tips, Tricks, Notes, Misc

• When testing iframes in Chrome, you sometimes must hit refresh twice in order to test your code changes. This is one
  reason that testing without iframes, using the `Data: colorized" wrapper is sometimes preferable.
• Sometimes toStateObject and fromStateObject need to manage private state, so must be declared in the type itself,
  see https://github.com/phetsims/phet-io/issues/107
• When navigating to wrappers, the easiest way to get to the whole wrapper suite is through the "wrapper index." After
  a while of testing it can be annoying to have the extra step: phetmarks --> index --> desired wrapper. Instead you
  can use phetmarks to launch any individual wrapper, just don't forget phetioValidateTandems=false until you
  are ready for it. Note that the wrapper index in the build version is at the top level of the build dir (build/phet-io/).

Two types of serialization

Data type serialization For example, numbers, strings, Vector2 instances fall into this category. These values
are instantiated by fromStateObject.

Reference type serialization For example, Nodes and Properties. For example, if a simulation has one heightProperty
that exists for the lifetime of the sim then when we save the state of the sim, we save the dynamic characteristics of
the heightProperty (rather than trying to serialize the entire list of listeners and phet-io metadata. Then the
PhET-iO library calls setValue() to update the dynamic characteristics of the heightProperty without dealing with
all of Property's many attributes. The static setValue methods on IO Types are automatically called by PhET-iO to
restore dynamic characteristics of reference-type serialized instances. Search for toStateObject in *IO.js files for examples.

Review and Publication

• The PhET-iO instrumentation should be code reviewed, create a new issue.
• The PhET-iO instrumentation should be QA tested, create a new issue.
• Update these instructions if you find them to be incomplete, inconsistent or incorrect.
• After publishing, add your instrumented simulation to the spreadsheet here: https://docs.google.com/spreadsheets/d/1pU9izdNQkd9vr8TvLAfXe_v68yh-7potH-y712FBPr8/edit#gid=0

Happy instrumenting!

[pixelzoom]

Raising priority to "high". Milestones section of the design document says "Features implemented by late September 2018". If the goal is to instrument this sim "as part of initial instrumentation" (as noted above), then we may have already missed that opportunity.

[pixelzoom]

Notes from 9/20/18 design meeting:

• toggle visibility of the equation that appears above sliders
• disable sliders individually
• disable pickers individually
• hide checkboxes individually
• hide point tools individually
• hide save and erase button as a group, but not individually
• hide complete accordion box
• hide complete control panel
• get full state of Quadratic model element
• instrument all view properties (GQViewProperties and subtypes)
• instrument drag handlers for manipulators
• instrument slider drag handlers
• do not instrument graph

[pixelzoom]

@amanda-phet @ariel-phet @kathy-phet FYI, notes about PhET-iO instrumentation are in the comment above. Let me know if I omitted anything.

[pixelzoom]

I'd like to review these requirements with @samreid before starting implementation. After stumbling across the "how to design PhET-iO features for a simulation document, I'm not confident that we have a solid plan for "amount of state that must be saved and restored".

[pixelzoom]

Instrumentation is partially completed. Compared to hookes-law, this is not going as smoothly, and talking a lot more time (6+ hours so far). Search for TODO #14 to identify remaining work.

[pixelzoom]

Worked on QuadraticIO a bit with @samreid, see above commits.

[pixelzoom]

Any estimate on when review will be completed? Graphing Quadratics needs to start dev testing on Friday 10/19 to stay on schedule. But it looks like there are a lot of checklist items that still need to be created.

[samreid]

I've unexpectedly had yesterday and today off, please coordinate with @ariel-phet @chrisklus and @zepumph to decide how to proceed.

[zepumph]

Today @chrisklus and @zepumph worked on this for a few hours. We went through all the code for the first screen (including sim specific common types). It would be safe to say that we checked all the boxes down to (not including) the Data Stream heading. The next three screens will likely go much faster since we spent most of our time in the sim common files (there is a lot of shared hierarchy in this sim).

[samreid]

I was unable to attend all meetings yesterday, is the conclusion that GQ is beginning dev testing for brand=phet today, then brand=phet-io will be for the future?

[chrisklus]

@samreid that's correct, though @pixelzoom would like us to keep up our momentum getting though the phet-io side of things next week.

[pixelzoom]

I was unable to attend all meetings yesterday, is the conclusion that GQ is beginning dev testing for brand=phet today, then brand=phet-io will be for the future?

Yes.

PhET-iO review is taking longer than anticipated, and there are a significant number of blocking issues.

After consultation with @ariel-phet, we decided that there was value in doing an initial dev test for brand=phet. So I initiated that yesterday, see phetsims/qa#211.

The goal is still to have PhET-iO instrumentation completed before final dev testing, code review, and RC testing.

[samreid]

Review complete, discovered questions and problems were described in new issues. There are these remaining steps, but they seem more relevant to the master checklist than part of the phet-io code review step:

• The PhET-iO instrumentation should be QA tested, create a new issue.
• Update these instructions if you find them to be incomplete, inconsistent or incorrect.
• After publishing, add your instrumented simulation to the spreadsheet here: https://docs.google.com/spreadsheets/d/1pU9izdNQkd9vr8TvLAfXe_v68yh-7potH-y712FBPr8/edit#gid=0

@pixelzoom back to you. Please let me know if you have any questions or comments about this review, or feel free to close if everything here is done and we can continue in the linked issues.

[pixelzoom]

I think we can close this now.