phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
10 stars 8 forks source link

Unbounded NumberProperties still put a range in the initial state with min and max as 0: #356

Closed samreid closed 3 years ago

samreid commented 3 years ago

In https://github.com/phetsims/phet-io/issues/1518, @zepumph said:

We found that unbounded NumberProperties still put a range in the initial state with min and max as 0:

https://github.com/phetsims/phet-io/blob/694c6642d8c9f8ba0b93e38bbf7f2418a42de382/api/natural-selection.json#L3081-L3085

 "range": {    "max": 0,    "min": 0  }, 
"rangePhetioID": "naturalSelection.introScreen.model.graphs.proportionsModel.proportionsGenerationProperty.rangeProperty", 
samreid commented 3 years ago

Here is the code that initialized the Property mentioned in that comment:

    // @public
    // Named proportionsGenerationProperty to distinguish it from the other 'generation' Properties in this sim.
    // See https://github.com/phetsims/natural-selection/issues/187
    this.proportionsGenerationProperty = new NumberProperty( 0, {
      numberType: 'Integer',
      range: new Range( 0, 0 ), // dynamically adjusted by calling setValueAndRange
      tandem: options.tandem.createTandem( 'proportionsGenerationProperty' ),
      phetioDocumentation: 'the generation whose data is displayed by the Proportions graph (integer)',
      phetioReadOnly: true // range is dynamic
    } );

Note that it does default to 0,0 initially.

samreid commented 3 years ago

In the commit, I made RangeIO use NumberIO for toStateObject/fromStateObject--should be better at positive and negative infinity. Ready for review.

pixelzoom commented 3 years ago

I’m still seeing buggy ranges in the NS API file, natural-selection.json. It shows a range with both min and max of zero, when in fact these Properties do not have a range because their values are unbounded. This problem was noted in last week’s PhET-iO meeting. For example:

"proportionsGenerationProperty": {
                "_data": {
                  "initialState": {
                    "numberType": "Integer",
                    "range": {
                      "max": 0,
                      "min": 0
                    },
                    "rangePhetioID": "naturalSelection.introScreen.model.graphs.proportionsModel.proportionsGenerationProperty.rangeProperty",
                    "value": 0
                  }
                },

4 instances of this in natural-selection.json. Search for "max": 0.

pixelzoom commented 3 years ago

@samreid and I looked at this on Zoom. It looks like this is OK, because NS is explicitly setting the initial range to new Range( 0, 0 ). In ProportionsModel.js, line 61:

    this.proportionsGenerationProperty = new NumberProperty( 0, {
      numberType: 'Integer',
      range: new Range( 0, 0 ), // dynamically adjusted by calling setValueAndRange
      tandem: options.tandem.createTandem( 'proportionsGenerationProperty' ),
      phetioDocumentation: 'the generation whose data is displayed by the Proportions graph (integer)',
      phetioReadOnly: true // range is dynamic
    } );
samreid commented 3 years ago

@zepumph and I reviewed this today and agreed it is ready to close, thanks!