Hard coded region tables are hard to maintain

[zepumph]

In GFL and GFLB we implemented a pattern where we hard coded numbers in a range that could then map to specific region strings (for dynamic description design). This pattern was then used in Molarity as well. I think that this should not be something that we propagate any further. In ISL we implemented those regions 6 months ago, and have already found two issues come about in the past month where we found these were out of date and broke the interactive descriptions.

One solution would be to just convert these regions into a polynomial regression that does a pretty good job of mapping the needs of the designed piecewise function. Online resources like https://planetcalc.com/5992/ could work for that.

I think that the preferred way to do this would be to just normalize the hard coded values to the range of the value being provided, so instead of the first value in GravityForceLabPositionDescriber.getDistanceIndex being 9.6, it would be 1, because it is the max of the range.

I put this issue in a11y-research because it feels like it applies to a pattern rather than a specific sim.

Tagging @jessegreenberg, @twant, and @terracoda for comment.

[twant]

@zepumph I agree that these are hard to maintain, and I like the idea of calculated regions. EDIT: just realized that I was calling hard-coded regions "single-value regions" but I think we mean the same things. I like normalizing those values, and I think we may need to be able to add custom ones somewhere in the middle of regions as well. For example, in Molarity we needed hard-coded regions right around saturation point.

[zepumph]

I experimented with a type that could manage all of this for us. It basically works by normalizing the values for us, by having a hard coded copied range along with the hard coded values. Then from there it can easily normalize the hard coded values and apply them to whatever value that has changed.

Here is the patch of the type and its usage:

``` class RegionsMap { /** * * @param {Range} actualRange * @param {Range} prototypeRange * @param {Array.<{value:number,region:string,[isSingleRegion:boolean]}>} values - should go from min to max in increasing order * @param regions */ constructor( actualRange, prototypeRange, values ) { // assert && assert( values.length === regions.length ); this.normalizedMap = values.map( ( valueObject, i ) => { return _.extend( valueObject, { value: this.normalize( valueObject.value, prototypeRange ), } ); } ); this.actualRange = actualRange; } normalize( value, range ) { return ( value - range.min ) / range.getLength(); } getRegion( value ) { const normalized = this.normalize( value, this.actualRange ); for ( let i = 0; i < this.normalizedMap.length; i++ ) { const valueElement = this.normalizedMap[ i ]; if ( valueElement.isSingleRegion && valueElement.value === normalized ) { return valueElement.region; } if ( i < this.normalizedMap.length - 1 && normalized < this.normalizedMap[ i + 1 ].value ) { return valueElement.region; } } } } const actualRange = new Range( 2, 12 ); const sizeRegionsMap = new RegionsMap( actualRange, // the actual range for the values in the sim new Range( 0, 10 ), // the range that the actualRange was when the following values were created. [ { 'region': 'least biggest', 'value': 0, 'isSingleRegion': true }, { 'region': 'not big', 'value': 0 // since this last one was a single region, this one is expected to be the same value, but exclusive of the value ¯\_(ツ)_/¯ }, { 'region': 'huh, I thought you said big', 'value': 2 }, { 'region': 'you call that big', 'value': 4 }, { 'region': 'less big', 'value': 6 }, { 'region': 'big', 'value': 8 }, { 'region': 'biggest', 'value': 10, 'isSingleRegion': true } ] ); ```

Here is a screenshot of some output


I think this is better than what we have currently.

I also think that it could be better, because it still depends on potentially stale values. This is a bummer because the "hard to maintain" piece is in part bad because of how you could be storing numbers in the code that don't really mean anything (since the "actualRange" has diverged). Basically the only alternative I could think of to this is to have the designer come up with the numbers as expected, and then, by hand, normalize them and input only the normalized values into the sim. That feels alright to me also, but still the normalized value may not hold a lot of useful info contextualized.

I'll keep thinking about it.

[terracoda]

@zepumph, I agree! I agree it would be ideal to be able to calculate the ranges based on the model. I would rather not provide numbers that can be calculated.

The ranges, whenever I have provided them, has been based on the idea that they are rough descriptions of the visual design.

What does this mean for https://github.com/phetsims/gravity-force-lab-basics/issues/182? I was talking to @emily-phet about the exact issue yesterday, not knowing this issue existed ;-)

[terracoda]

If we take GFLB as an example, we can't just take the range of force (0.7 N - 3949.2 N) and divide 3948.5 N into 7 equal regions. In this sim, the regions on the smaller side have smaller ranges.

As designer without a science background, I have a lot of trouble figuring out how to estimate or even calculate a range for each region.

Any help we can get with this will be greatly appreciated.

[terracoda]

The https://planetcalc.com/5992/ tool looks very interesting!

[zepumph]

@jessegreenberg and I were discussing my prototype in the details above, and we think it would be worth committing. I think scenery-phet makes the most sense. Let's start with RegionMapper.

[zepumph]

As designer without a science background, I have a lot of trouble figuring out how to estimate or even calculate a range for each region.

I think that this plays into discussion we have had before about having the dev and designer meet early in the process to discover what model Properties are at the designer's disposal. Ranges of the Properties seem important to convey as well.

[terracoda]

@zepumph, I agree, and we did meet early in the design process for GFL. We had determined that there was some "visual hollywooding" on the force vectors, but no much else.

To help with my understanding of the model either @zepumph or @jessegreenberg plotted two graphs (see design doc) for me. These graphs are now out of date due to the bug found in ISLC, but all said, I think we are doing the right stuff to prepare the description design process.

I don't know if there is an easy way to automatically map the model to a described scale whose ranges by design are much fewer than the full quantitative range.

If there is a way to automatically map described ranges evenly across the full range, that could be our starting point, and then we could, if necessary, "hollywood" the descriptions together with the visuals with the same design goals and same learning goals in mind.

[zepumph]

If there is a way to automatically map described ranges evenly across the full range, that could be our starting point, and then we could, if necessary, "hollywood" the descriptions together with the visuals with the same design goals and same learning goals in mind.

After hearing this, I think I have changed my mind about the necessity for a "RegionMapper" type. It sounds like the preferred mechanism is to try to map it to a function. I'm less interested in building out something that is meant to be the fallback method.

Instead I think what we can do is just normalize all the values hard coded in GFL and Molarity describers. That way they are future proof.

Over the past week, I have been thinking about how to create this generic region mapper, and I kept feeling like I was over-engineering it. Perhaps this is the best output. I want to talk to @jessegreenberg first before proceeding.

UPDATE: Here is my working copy so I can get back to it if we do more about a region mapper:

```diff Index: inverse-square-law-common/js/view/describers/ForceDescriber.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- inverse-square-law-common/js/view/describers/ForceDescriber.js (revision 7dbdd1d3c04b44f2cad75c2a46181d5efa82cbef) +++ inverse-square-law-common/js/view/describers/ForceDescriber.js (date 1571760185997) @@ -15,6 +15,8 @@ const ISLCA11yStrings = require( 'INVERSE_SQUARE_LAW_COMMON/ISLCA11yStrings' ); const ISLCDescriber = require( 'INVERSE_SQUARE_LAW_COMMON/view/describers/ISLCDescriber' ); const merge = require( 'PHET_CORE/merge' ); + const Range = require( 'DOT/Range' ); + const RegionMapper = require( 'SCENERY_PHET/accessibility/RegionMapper' ); const ScientificNotationNode = require( 'SCENERY_PHET/ScientificNotationNode' ); const StringUtils = require( 'PHETCOMMON/util/StringUtils' ); @@ -106,6 +108,9 @@ options = merge( { units: unitsNewtonsString, + // To map size and pull effort strings. See RegionMapper.js for schema. + regionMapValues: [], + // in some scenarios, the force units change. convertForce allows subtypes to define conversion behavior // integrates with forceValueToString for necessary conversions (e.g. 300000000 -> 3) // always takes place before forceValueToString @@ -133,6 +138,10 @@ this.forceValueToString = options.forceValueToString; this.convertForce = options.convertForce; + const modelForceRange = new Range( model.getMinForce(), model.getMaxForce() ); + assert && assert( SIZE_STRINGS.length === options.regionMapValues, 'should be same number of regions' ); + const sizeRegionObjects = options.regionMapValues.this.sizeRegionMapper = new RegionMapper(); + // @private {number} - // 1 -> growing, 0 -> no change, -1 -> shrinking this.vectorChangeDirection = 0; this.forceVectorsString = options.forceVectorsString; Index: gravity-force-lab/js/gravity-force-lab/view/describers/GravityForceLabForceDescriber.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- gravity-force-lab/js/gravity-force-lab/view/describers/GravityForceLabForceDescriber.js (revision ac50781e4c00df4a6dc5e60909b90908d7b8c520) +++ gravity-force-lab/js/gravity-force-lab/view/describers/GravityForceLabForceDescriber.js (date 1571759803049) @@ -9,9 +9,10 @@ 'use strict'; // modules - const gravityForceLab = require( 'GRAVITY_FORCE_LAB/gravityForceLab' ); const ForceDescriber = require( 'INVERSE_SQUARE_LAW_COMMON/view/describers/ForceDescriber' ); + const gravityForceLab = require( 'GRAVITY_FORCE_LAB/gravityForceLab' ); const GravityForceLabA11yStrings = require( 'GRAVITY_FORCE_LAB/gravity-force-lab/GravityForceLabA11yStrings' ); + const Range = require( 'DOT/Range' ); const Util = require( 'DOT/Util' ); // strings @@ -25,6 +26,15 @@ const convertForceToMicronewtons = force => { return Util.toFixedNumber( force * MICRO_CONVERSION_FACTOR, 6 ); }; + const REGION_MAP_VALUES = [ + { value: 0, singleValueRegion: true }, + { value: 0.166852 }, + { value: 2.206307 }, + { value: 4.412615 }, + { value: 8.687337 }, + { value: 19.856768 }, + { value: 35.300920 } + ]; class GravityForceLabForceDescriber extends ForceDescriber { @@ -51,7 +61,9 @@ return ForceDescriber.getForceInScientificNotation( convertedForce, 2 ); } return convertedForce + ''; - } + }, + + regionMapValues: REGION_MAP_VALUES }; super( model, object1Label, object2Label, positionDescriber, options ); Index: scenery-phet/js/accessibility/RegionMapper.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- scenery-phet/js/accessibility/RegionMapper.js (date 1571769246732) +++ scenery-phet/js/accessibility/RegionMapper.js (date 1571769246732) @@ -0,0 +1,80 @@ +// Copyright 2017-2019, University of Colorado Boulder + +/** + * + * @author Michael Kauzmann (PhET Interactive Simulations) + */ + +define( require => { + 'use strict'; + + const sceneryPhet = require( 'SCENERY_PHET/sceneryPhet' ); + + class RegionMapper { + + /** + * + * @param {Range} range + * @param {Array.<{value:number,[isSingleRegion:boolean]}>} values - should go from min to max in increasing order + */ + constructor( range, values ) { + + this.range = range; + + } + + normalize( value, range = this.range ) { + return; + } + + getRegionIndex( value ) { + const normalized = this.normalize( value ); + for ( let i = 0; i < this.normalizedMap.length; i++ ) { + const valueElement = this.normalizedMap[ i ]; + + if ( valueElement.isSingleRegion && valueElement.value === normalized ) { + return valueElement.region; + } + + // TODO: support case where there is less, but from a previous single region value (like i-1) + if ( i < this.normalizedMap.length - 1 && normalized < this.normalizedMap[ i + 1 ].value ) { + return valueElement.region; + } + } + } + } + + // + // const actualRange = new Range( 2, 12 ); + // const sizeRegionMap = new RegionMap( + // actualRange, // the actual range for the values in the sim + // new Range( 0, 10 ), // the range that the actualRange was when the following values were created. + // [ { + // 'region': 'least biggest', + // 'value': 0, + // 'isSingleRegion': true + // }, { + // 'region': 'not big', + // 'value': 0 // since this last one was a single region, this one is expected to be the same value, but exclusive of the value ¯\_(ツ)_/¯ + // }, { + // 'region': 'huh, I thought you said big', + // 'value': 2 + // }, { + // 'region': 'you call that big', + // 'value': 4 + // }, { + // 'region': 'less big', + // 'value': 6 + // }, { + // 'region': 'big', + // 'value': 8 + // }, { + // 'region': 'biggest', + // 'value': 10, + // 'isSingleRegion': true + // } + // ] ); + + + return sceneryPhet.register( 'RegionMapper', RegionMapper ); +} ); ```

[zepumph]

After discussing more with @jessegreenberg, we think that it makes sense to proceed with a formalized type to encapsulate this pattern.

Furthermore, we thought that we may be able to treat this more like math, where we have a a piecewise function of flat x=y steps, and then worked out whether points are inclusive or exclusive to each function (or their own single value step). I'm not sure how easy it would be to outfit this logic into PiecewiseLinearFunction, or to supertype it. I'll investigate.

[terracoda]

@zepumph, just a note that I still don't hear "closest" when the mass spheres are closest see closed issue https://github.com/phetsims/gravity-force-lab/issues/178

I don't need to re-open https://github.com/phetsims/gravity-force-lab/issues/178, do I?

[zepumph]

I don't need to re-open phetsims/gravity-force-lab#178, do I?

You shouldn't need to.

Sorry for the delay here.

After @jessegreenberg and my previous conversation I tried to work out a formalized type, but never got a lot of traction. What I kept coming back to (in part through conversation with other devs), is that you will always need to give normalized values to the type, so the value of a formalized Type is actually quite limited. Over in GFL we ended up normalizing the values from the model before comparing it to the normalized, but hard coded values in the function. For example see GravityForceLabForceDescriber.getForceVectorIndex.

This is now 2 months out of date (since we did https://github.com/phetsims/molarity/issues/183), and I don't think I will work on this anymore. I'm going to close this, and we can reinvestigate if in future sims this pattern seems more fruitful. Anyone can reopen if they would like to discuss more.

[terracoda]

I do get "closest" in both GFL regular and Basics the last time I checked.