How to create an instrumented DynamicProperty of type number with a Range?

[pixelzoom]

I have a natural place to use a bidirectional DynamicProperty in Fourier, and I'm running into a couple of problems:

(1) My DynamicProperty needs to have type 'number' and a Range, so that it can used with NumberControl. I see no way to define a range for DynamicProperty.

(2) My DynamicProperty needs to be PhET-iO instrumented and editable from Studio. I see no DynamicPropertyIO, so no way to specify phetioType: DynamicPropertyIO( NumberIO ).

Any insights, examples, etc. would be appreciated.

@jonathanolson is the author of DynamicProperty, and seems to have used it the most. I'll start by assigning to him, and the PhET-iO team.

[pixelzoom]

Here's my specific problem. In WavePacket.js, I have 2 Properties: standardDeviationProperty and inverseStandardDeviationProperty. The relationship is:

inverseStandard = 1 / standardDeviation

... and both Properties need to be editable, in the sim's UI and in Studio. I'm currently using 2 NumberProperty instances, and handling my own synchronization.

Here's where I got to with DynamicProperty before bailing. The TODO comments indicate where I got stuck.

    // @public
    this.standardDeviationProperty = new NumberProperty( 3 * Math.PI, {
      range: new Range( 1, 4 * Math.PI ),
      tandem: options.tandem.createTandem( 'standardDeviationProperty' ),
      phetioDocumentation: 'Standard deviation, a measure of the wave packet width. ' +
                           'In the space domain, this is \u03c3<sub>k</sub>, in rad/m. ' +
                           'In the time domain, this is \u03c3<sub>\u03c9</sub>, in rad/ms.'
    } );

    // @public
    this.inverseStandardDeviationProperty = new DynamicProperty( new Property( this.standardDeviationProperty ), {
      bidirectional: true,
      reentrant: true, // necessary because bidirectional:true
      map: value => 1 / value, // standardDeviation => inverseStandardDeviation
      inverseMap: value => 1 / value, // inverseStandardDeviation => standardDeviation
      //TODO range: new Range( 1 / this.standardDeviationProperty.range.max, 1 / this.standardDeviationProperty.range.min,
      //TODO phetioType: ???,
      tandem: options.tandem.createTandem( 'this.inverseStandardDeviationProperty' ),
      phetioDocumentation: 'Inverse standard deviation, a measure of the wave packet width. ' +
                           'In the space domain, this is \u03c3<sub>x</sub>, in m/rad. ' +
                           'In the time domain, this is \u03c3<sub>t</sub>, in ms/rad.'
    } );

[samreid]

I am less experienced with DynamicProperty and not sure what's best here.

I'm currently using 2 NumberProperty instances, and handling my own synchronization.

That sounds advantageous, unless there are problems with that pattern?

[zepumph]

unless there are problems with that pattern?

I'd probably call it more of a work around then a pattern. We have a pattern and library type set up to support this synchronization for us (DynamicProperty), but PhET-iO is not yet set up to support it.

Brainstorming here, I could see a way to support an instrumented DynamicProperty that is constrained in that its value Property never changes. If we can assert that, (that the intermediate Property never changes), then we can likely support DynamicProperty very easily, with a PropertyIO(ParameterTypeIO) phetioType.

I got here by experimenting with a forwarding IOType, but then realized that the PhET-iO-ness of the DynamicProperty actually doesn't need to know anything about the other Property, because the map/inverseMap functions will be the same in the the upstream and downstream sims. Thus. I recommend this for the phetioType above: PropertyIO( NumberIO ). For the range, it will get a bit messy, but that is only needed for studio slider control support, and not for getting things up and running as an mvp.

This patch is working in studio, but doesn't have a studio control just yet because of serializing with PropertyIO( NumberIO ) instead of NumberProperty.

```diff Index: fourier-making-waves/js/wavepacket/view/WavePacketControlPanel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/fourier-making-waves/js/wavepacket/view/WavePacketControlPanel.js b/fourier-making-waves/js/wavepacket/view/WavePacketControlPanel.js --- a/fourier-making-waves/js/wavepacket/view/WavePacketControlPanel.js (revision 9599bcc61de41cb4a6620d80ed2fbba603134c83) +++ b/fourier-making-waves/js/wavepacket/view/WavePacketControlPanel.js (date 1627915564222) @@ -27,12 +27,13 @@ import fourierMakingWaves from '../../fourierMakingWaves.js'; import fourierMakingWavesStrings from '../../fourierMakingWavesStrings.js'; import WavePacketModel from '../model/WavePacketModel.js'; -import StandardDeviationControl from './StandardDeviationControl.js'; -import ConjugateStandardDeviationControl from './ConjugateStandardDeviationControl.js'; import CenterControl from './CenterControl.js'; import ComponentSpacingControl from './ComponentSpacingControl.js'; +import ConjugateStandardDeviationControl from './ConjugateStandardDeviationControl.js'; +import StandardDeviationControl from './StandardDeviationControl.js'; import WavePacketSymbolsDialog from './WavePacketSymbolsDialog.js'; import WidthIndicatorsCheckbox from './WidthIndicatorsCheckbox.js'; +import Range from '../../../../dot/js/Range.js'; // constants const VERTICAL_SPACING = 7; @@ -258,7 +259,7 @@ assert && AssertUtils.assertEnumerationPropertyOf( domainProperty, Domain ); assert && assert( standardDeviationProperty instanceof NumberProperty ); - assert && assert( conjugateStandardDeviationProperty instanceof NumberProperty ); + assert && assert( conjugateStandardDeviationProperty.range instanceof Range ); assert && AssertUtils.assertPropertyOf( widthIndicatorsVisibleProperty, 'boolean' ); options = merge( {}, FMWConstants.VBOX_OPTIONS, { Index: fourier-making-waves/js/common/view/FMWNumberControl.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/fourier-making-waves/js/common/view/FMWNumberControl.js b/fourier-making-waves/js/common/view/FMWNumberControl.js --- a/fourier-making-waves/js/common/view/FMWNumberControl.js (revision 9599bcc61de41cb4a6620d80ed2fbba603134c83) +++ b/fourier-making-waves/js/common/view/FMWNumberControl.js (date 1627915681202) @@ -6,10 +6,10 @@ * @author Chris Malley (PixelZoom, Inc.) */ -import NumberProperty from '../../../../axon/js/NumberProperty.js'; import NumberControl from '../../../../scenery-phet/js/NumberControl.js'; import PressListener from '../../../../scenery/js/listeners/PressListener.js'; import fourierMakingWaves from '../../fourierMakingWaves.js'; +import Range from '../../../../dot/js/Range.js'; class FMWNumberControl extends NumberControl { @@ -19,8 +19,7 @@ * @param {Object} [options] */ constructor( title, numberProperty, options ) { - assert && assert( numberProperty instanceof NumberProperty ); - assert && assert( numberProperty.range ); + assert && assert( numberProperty.range instanceof Range ); super( title, numberProperty, numberProperty.range, options ); Index: axon/js/DynamicProperty.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/DynamicProperty.js b/axon/js/DynamicProperty.js --- a/axon/js/DynamicProperty.js (revision fa529a848da800183b03818033ff4027658bfaea) +++ b/axon/js/DynamicProperty.js (date 1627915896570) @@ -152,6 +152,11 @@ this.derive = derive; this.map = map; + // TODO: wouldn't typescript be nice to have interfaces for number-like-Properties?!?!?! + if ( options.range ) { + this.range = options.range; + } + // @private {function} this.inverseMap = typeof options.inverseMap === 'string' ? _.property( options.inverseMap ) : options.inverseMap; Index: fourier-making-waves/js/wavepacket/view/ConjugateStandardDeviationControl.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/fourier-making-waves/js/wavepacket/view/ConjugateStandardDeviationControl.js b/fourier-making-waves/js/wavepacket/view/ConjugateStandardDeviationControl.js --- a/fourier-making-waves/js/wavepacket/view/ConjugateStandardDeviationControl.js (revision 9599bcc61de41cb4a6620d80ed2fbba603134c83) +++ b/fourier-making-waves/js/wavepacket/view/ConjugateStandardDeviationControl.js (date 1627915564222) @@ -7,7 +7,7 @@ */ import DerivedProperty from '../../../../axon/js/DerivedProperty.js'; -import NumberProperty from '../../../../axon/js/NumberProperty.js'; +import Range from '../../../../dot/js/Range.js'; import Utils from '../../../../dot/js/Utils.js'; import merge from '../../../../phet-core/js/merge.js'; import AssertUtils from '../../../../phetcommon/js/AssertUtils.js'; @@ -36,7 +36,7 @@ constructor( domainProperty, conjugateStandardDeviationProperty, options ) { assert && AssertUtils.assertEnumerationPropertyOf( domainProperty, Domain ); - assert && assert( conjugateStandardDeviationProperty instanceof NumberProperty ); + assert && assert( conjugateStandardDeviationProperty.range instanceof Range ); assert && assert( conjugateStandardDeviationProperty.range ); options = merge( {}, FMWConstants.WAVE_PACKET_NUMBER_CONTROL_OPTIONS, { Index: fourier-making-waves/js/wavepacket/model/WavePacket.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/fourier-making-waves/js/wavepacket/model/WavePacket.js b/fourier-making-waves/js/wavepacket/model/WavePacket.js --- a/fourier-making-waves/js/wavepacket/model/WavePacket.js (revision 9599bcc61de41cb4a6620d80ed2fbba603134c83) +++ b/fourier-making-waves/js/wavepacket/model/WavePacket.js (date 1627915407803) @@ -14,7 +14,9 @@ */ import DerivedProperty from '../../../../axon/js/DerivedProperty.js'; +import DynamicProperty from '../../../../axon/js/DynamicProperty.js'; import NumberProperty from '../../../../axon/js/NumberProperty.js'; +import Property from '../../../../axon/js/Property.js'; import Range from '../../../../dot/js/Range.js'; import NumberIO from '../../../../tandem/js/types/NumberIO.js'; import fourierMakingWaves from '../../fourierMakingWaves.js'; @@ -52,9 +54,16 @@ // @public // See https://github.com/phetsims/fourier-making-waves/issues/105#issuecomment-889386852 for name decision. - this.conjugateStandardDeviationProperty = new NumberProperty( 1 / this.standardDeviationProperty.value, { + this.conjugateStandardDeviationProperty = new DynamicProperty( new Property( this.standardDeviationProperty ), { + bidirectional: true, + reentrant: true, // necessary because bidirectional:true + map: value => 1 / value, // standardDeviation => inverseStandardDeviation + inverseMap: value => 1 / value, // inverseStandardDeviation => standardDeviation range: new Range( 1 / this.standardDeviationProperty.range.max, 1 / this.standardDeviationProperty.range.min ), + + // phet-io tandem: options.tandem.createTandem( 'conjugateStandardDeviationProperty' ), + phetioType: Property.PropertyIO( NumberIO ), phetioDocumentation: 'This Property and standardDeviationProperty are a conjugate pair, ' + 'where conjugateStandardDeviation = 1 / standardDeviation.' + 'They are both measures of the wave packet width. ' + @@ -62,28 +71,6 @@ 'In the time domain, this is \u03c3<sub>t</sub> in ms.' } ); - // conjugateStandardDeviationProperty seems like a natural place to use DynamicProperty. But since we need to - // control it with a NumberControl, it needs to be a {Property.} with a Range. We also need to control - // it from Studio, which also requires a phetioType, and there's currently no DynamicPropertyIO. So this next - // bit of code does the work that a bidirectional DynamicProperty would do - it keeps standardDeviation and - // conjugateStandardDeviationProperty synchronized. And unlike DynamicProperty, it avoids reentrant behavior, - // so neither Property requires reentrant:true. See https://github.com/phetsims/axon/issues/358 - let isSynchronizing = false; - this.standardDeviationProperty.lazyLink( standardDeviation => { - if ( !isSynchronizing ) { - isSynchronizing = true; - this.conjugateStandardDeviationProperty.value = 1 / standardDeviation; - isSynchronizing = false; - } - } ); - this.conjugateStandardDeviationProperty.lazyLink( conjugateStandardDeviation => { - if ( !isSynchronizing ) { - isSynchronizing = true; - this.standardDeviationProperty.value = 1 / conjugateStandardDeviation; - isSynchronizing = false; - } - } ); - // @public this.widthProperty = new DerivedProperty( [ this.standardDeviationProperty ], ```

Note this is not yet a complete instrumentation of DynamicProperty, but perhaps our first foot in the door for tackling it.

---

Side note: Here is an IOType I played with but feel like we don't need to support at this time:

```js const cache = new Map(); /** * An observable Property that triggers notifications when the value changes. * This caching implementation should be kept in sync with the other parametric IO Type caching implementations. * @param {IOType} propertyParameterType * @returns {IOType} */ DynamicProperty.DynamicPropertyOfStaticPropertyIO = propertyParameterType => { assert && assert( propertyParameterType, 'PropertyIO needs parameterType' ); if ( !cache.has( propertyParameterType ) ) { assert && assert( propertyParameterType.typeName.startsWith( 'PropertyIO' ), 'DynamicPropertyOfStaticPropertyIO only supports a Property parameter type' ); cache.set( propertyParameterType, new IOType( `DynamicPropertyOfStaticPropertyIO<${propertyParameterType.typeName}>`, { valueType: DynamicProperty, documentation: '', // TODO parameterTypes: [ propertyParameterType ], toStateObject: property => { return propertyParameterType.toStateObject( property ); }, applyState: ( property, stateObject ) => { propertyParameterType.applyState( property, stateObject ); }, stateSchema: propertyParameterType.stateSchema } ) ); } return cache.get( propertyParameterType ); }; ```

[zepumph]

@samreid, can you please comment on how close this pattern might be to production-ready. Main concerns:

1. DynamicProperty doesn't support range at this time.
2. If the goal is to have a studio control, then perhaps we need to serialize with NumberPropertyIO to get a range?
3. Can we somehow assert that when using a PropertyIO IOType in DynamicProperty, that we make sure that the intermediate Property cannot change? At least for right now.

[pixelzoom]

@zepumph said:

unless there are problems with that pattern? I'd probably call it more of a work around then a pattern. We have a pattern and library type set up to support this synchronization for us (DynamicProperty), but PhET-iO is not yet set up to support it.

Yes, that was my point. Like I said, I have this working. But the fact that DynamicProperty doesn't have PhET-iO support seems like it needed addressing.

Note this is not yet a complete instrumentation of DynamicProperty, but perhaps our first foot in the door for tackling it.

This looks promising. But I had (have) many of the same questions as @samreid. So... For Fourier, I will likely stick with my workaround, unless a complete solution is available before August 24 (the "feature complete" milestone for Fourier). And reminder: Fourier 1.0 will not be published for PhET-iO brand.

[zepumph]

unless a complete solution

By incomplete I was talking about DynamicProperty's ability to swap out the intermediate Property (something PhET-iO is not well set up for at all because of dynamicProperties). For the fourier usage, I believe that we could get something working very well.

Oh! I have a new idea @samreid. Why don't we create a DynamicPropertyIO type, and all it does is serialize is the exact thing my patch above does with one added piece. It can assert that the intermediateProperty is instrumented, and use referenceIO to serialize it. Let me write out an untested scrap for you.

```diff Index: js/DynamicProperty.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/DynamicProperty.js b/js/DynamicProperty.js --- a/js/DynamicProperty.js (revision fa529a848da800183b03818033ff4027658bfaea) +++ b/js/DynamicProperty.js (date 1627918833383) @@ -97,9 +97,15 @@ */ import merge from '../../phet-core/js/merge.js'; +import Tandem from '../../tandem/js/Tandem.js'; +import IOType from '../../tandem/js/types/IOType.js'; +import NullableIO from '../../tandem/js/types/NullableIO.js'; import axon from './axon.js'; import Property from './Property.js'; +const VALUE_PROPERTY_PROPERTY_NULL = 'VALUE_PROPERTY_PROPERTY_NULL'; + + class DynamicProperty extends Property { /** @@ -197,6 +203,10 @@ */ onPropertyPropertyChange( value, oldValue, innerProperty ) { + if ( innerProperty && Tandem.VALIDATION && this.isPhetioInstrumented() ) { + assert && assert( innerProperty.isPhetioInstrumented() ); + } + // If the value of the inner Property is already the inverse of our value, we will never attempt to update our // own value in an attempt to limit "ping-ponging" cases mainly due to numerical error. Otherwise it would be // possible, given certain values and map/inverse, for both Properties to toggle back-and-forth. @@ -322,5 +332,62 @@ } } + +const cache = new Map(); + +/** + * An observable Property that triggers notifications when the value changes. + * This caching implementation should be kept in sync with the other parametric IO Type caching implementations. + * @param {IOType} parameterType + * @returns {IOType} + */ +DynamicProperty.DynamicPropertyIO = parameterType => { + assert && assert( parameterType, 'PropertyIO needs parameterType' ); + + if ( !cache.has( parameterType ) ) { + + cache.set( parameterType, new IOType( `DynamicPropertyIO<${parameterType.typeName}>`, { + valueType: DynamicProperty, + superType: PropertyIO( parameterType ), + documentation: '', // TODO https://github.com/phetsims/axon/issues/358 + parameterTypes: [ parameterType ], + toStateObject: property => { + const stateObject = PropertyIO( parameterType ).toStateObject( property.value ); + if ( property.valuePropertyProperty ) { + stateObject.propetyID = NullableIO( ReferenceIO( Property.PropertyIO( ValueIO ) ) ).toStateObject( property.valuePropertyProperty.value ); + } + else { + stateObject.propetyID = VALUE_PROPERTY_PROPERTY_NULL; + } + return stateObject; + }, + applyState: ( property, stateObject ) => { + + if ( stateObject.phetioID !== VALUE_PROPERTY_PROPERTY_NULL ) { + + // Set to the new Property reference since it should exist and be instrumented + // My thought is to do this first so that the apply state, if bidirectional, can support setting the new value on the right Property instance. + // TODO: is this order of operations going to be problematic with deferred Properties? https://github.com/phetsims/axon/issues/358 + property.valuePropertyProperty.value = NullableIO( ReferenceIO( Property.PropertyIO( ValueIO ) ) ).fromStateObject( stateObject.propertyID ); + + } + else { + // the valuePropertyProperty is null, and likely it always has been? TODO: do we need to support setting this property?!?!?! https://github.com/phetsims/axon/issues/358 + } + + // TODO: pretty sure this won't work out of the box https://github.com/phetsims/axon/issues/358 + PropertyIO( parameterType ).applyState( property, stateObject ); + }, + stateSchema: { + propertyID: NullableIO( ReferenceIO( Property.PropertyIO( ValueIO ) ) ), + value: parameterType.stateSchema + } + } ) ); + } + + return cache.get( parameterType ); +}; + + axon.register( 'DynamicProperty', DynamicProperty ); export default DynamicProperty; \ No newline at end of file

[samreid]

I'm having trouble evaluating whether it is acceptable to require that instrumented DynamicProperty instances wrap Property instance which are also instrumented. Is that a natural fit, or are many of the wrapped Property instances implementation details which we would hope to leave uninstrumented? Even if so, would we be OK adding instrumentation to them, and making them be non-featured?

[zepumph]

I think that seems like the easiest path forward, but it also feels like a total pain. I feel a bit like being graceful, and assuming that if you pass in an uninstrumented Property to an instrumented DynamicProperty, it is because that Property doesn't matter, and also doesn't change (therefore it doesn't need serialization). I don't think it is too clever, so long as we can explain how to support both cases.

[samreid]

It sounds reasonable to me, but wondering if we should touch base in a mini-meeting between myself and @jonathanolson and @zepumph. Perhaps as a breakout room at dev meeting?

[zepumph]

I quite like that. Added it!

[samreid]

I'll mark as on hold, but only until @jonathanolson @zepumph and I can discuss, perhaps at an upcoming Thursday meeting. This helps filter it out of my "to do" list.

[jonathanolson]

I ran into needing this to port something in Density/Buoyancy.

Defined initial types and type guards in NumberProperty: https://github.com/phetsims/axon/blob/7f0b33caffc1ebc5a19087096dfe4012b7f152f6/js/NumberProperty.ts#L44-L59

With asRanged() and friends that gives an initial check + cast in a safe-ish way: https://github.com/phetsims/axon/blob/7f0b33caffc1ebc5a19087096dfe4012b7f152f6/js/NumberProperty.ts#L313-L341

I added the long-needed MappedProperty (basically a DynamicProperty for a single Property, used for map/inverseMap), and a RangedMappedProperty that has a hard range (and implements RangedProperty).

Additionally for my needs, I implemented UnitConversionProperty, which is meant for converting units (number => number). It also converts the rangeProperty if needed (I was doing this manually before).

Thus a before (without type safety):

    // For unit conversion
    const volumeProperty = new DynamicProperty( new Property( model.volumeProperty ), {
      map: cubicMeters => 1000 * cubicMeters,
      inverseMap: liters => liters / 1000,
      bidirectional: true
    } );
    volumeProperty.range = new Range( model.volumeProperty.range.min * 1000, model.volumeProperty.range.max * 1000 );

and after:

    // For unit conversion, cubic meters => liters
    const volumeProperty = new UnitConversionProperty( model.volumeProperty, {
      factor: 1000
    } ).asRanged();

where afterwards it has a guaranteed range to be used elsewhere (it types as RangedProperty).

[jonathanolson]

Also, UnitConverstionProperty can do non-linear things. Just provide map/inverseMap instead of factor (and it will still handle the range, typing, etc.)

[pixelzoom]

This all sounds reasonable, and nice to know that these exist. Code looks clean, and it's nice that there are examples in the docs. I don't have time for a detailed test-drive right now, so may have some feedback in the future. Self unassigning.

[jonathanolson]

@samreid I was assigned here. Is there anything I should do?

[samreid]

It's unclear to me whether we have addressed all of @pixelzoom requests, or if we will need something like DynamicPropertyIO as discussed in https://github.com/phetsims/axon/issues/358#issuecomment-891127785.

[jonathanolson]

I definitely don't know what the desired behavior would be for a DynamicPropertyIO.

[pixelzoom]

This issue is so old now that it's moot for Fourier. And my attempts to use DynamicProperty have generally not gone well, so I'll try my best to avoid it in the future. So I'm unassigning myself and unsubscribing from this issue. I'll leave it up to the PhET-iO team to decide whether it's worth the effort to figure this out. I don't care whether you close this issue, or continue to invesigate.

[marlitas]

Discussed with @samreid, and he recommended that this issue be closed since we do not currently needs this functionality for DynamicProperty. Can be re-opened when that time comes.