KickDistributionStrategy assumes 15 distance values

marlitas commented 7 months ago

While working on https://github.com/phetsims/mean-share-and-balance/issues/152, I noticed that the RIGHT_SKEWED_DATA array in KickDistributionStrategy is hard coded to assume 15 possible values (1-15). This does not work well with MSAB which has 11 possible values (0-10). Currently this constant is used by phet-io in soccer-common, so adjusting this to work for MSAB looks like it may affect CAV as well.

A few questions:

Are PhET-iO clients able to set distribution for soccer balls in MSAB the same way they have been allowed in CAV?
If we want to keep the data sets hard coded we can do so in each repo's constants file. This means that the phet-io documentation will need to move to subclass usages. This is probably the most straightforward solution.
If we don't want to move documentation, we can find a way to generate the distribution through code based on the size of the probability array needed. Would CAV be okay with a RIGHT/LEFT_SKEWED_DATA set that is generated at start up? This would also require us to change wording in the current phet-io documentation which is displaying the constants as the values clients can assume exist.

@amanda-phet, if the answer to the first question is no, then this is really straightforward and we don't even need to talk through the other questions.

amanda-phet commented 7 months ago

I thought this through with @marlitas and @catherinecarter and I do think we want clients to be able to set distribution in the same was as CaV.

@marlitas over to you!

marlitas commented 7 months ago

This has been addressed, but there is still a TODO regarding hard coded examples in the documentation... I'm assuming we'll want to pass this through options as well, but I'll hold off until we know what that is.

Current documentation:

amanda-phet commented 2 months ago

For Mean: Share and Balance:

Underlying distribution for skewed right: [8,12,15,18,12,2,2,2,2,2,2]

Update PhET-iO documentation examples:

{
   "type": "probabilityByDistance",
   "values": [0,1,3,5,7,3,1,0,0,0,1],
   "skewType": null
}

 {
   "type": "distanceByIndex",
   "values": [5,9,10,2,7,3,4],
   "skewType": null
}

marlitas commented 2 months ago

I accidentally committed to https://github.com/phetsims/mean-share-and-balance/issues/9: https://github.com/phetsims/soccer-common/commit/7b91dac3fdc71f82925deee16998fc8edbd2b7fd

But this is done and ready for @amanda-phet review. Feel free to close if all looks well.

amanda-phet commented 2 months ago

Looks good! Thanks!

phetsims / soccer-common

KickDistributionStrategy assumes 15 distance values #9