search

phetsims / unit-rates

"Unit Rates" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 2 forks source link

User reported issue: Green candy price #202

Closed oliver-phet closed 6 years ago

oliver-phet commented 6 years ago

Problem description: Unit Rates Shopping simulation using green candy – price on scale doesn’t match price on number line. Using price on scale, you need to round up to the nearest ten cents (rather than rounding up to the next cent) to get the simulation to accept the unit rate as correct. Using the price on the number line, no rounding is needed.

Steps to reproduce: This mismatch between scale and number line occurs whether you drag one, two, three or four bags onto the scale. In each case, the price that shows on the scale is one cent less than the price that shows on the number line.

amanda-phet commented 6 years ago

This is pretty serious.. I just checked it out and indeed the challenge questions are using the values on the number line, not the scale. It is very strange that the number line values don't match the scale!

arouinfar commented 6 years ago

I reviewed the latest, and the price on the scale is incorrect for some bags of candy (see below). I checked all of Questions and major tick marks on the DNL for all candy types, and everything is correct. The problem seems to be the price on the scale for these five configurations of candy.

Item Price on Scale (incorrect) Price on DNL (correct)
1 bag green candy $2.45 $2.46
2 bags green candy $4.91 $4.92
3 bags green candy $7.37 $7.38
4 bags green candy $9.83 $9.84
3 bags red candy $3.41 $3.42

It looks like this behavior may have been introduced in dev.60 (dev.59 and the few earlier versions that I checked appear to be okay).

Assigning to @pixelzoom for investigation.

pixelzoom commented 6 years ago

@arouinfar

(1) Have you verified this part of the user's report?

Using price on scale, you need to round up to the nearest ten cents (rather than rounding up to the next cent) to get the simulation to accept the unit rate as correct.

(2) Have you verified that this is a problem only for candy?

pixelzoom commented 6 years ago

OK, I understand (1) above. They mean entering a value in the keypad. And the fact that it rounds up to the nearest ten cents is a symptom of the scale being off by 0.01.

Still interested if this is a problem only for candy. I seriously doubt it, since all items use the same scale.

1.0.0-dev.59 and 1.0.0-dev.60 were both published on 2/22/17. It looks like that was when I was adding the 3rd (grayed out) decimal place to the cost display on the scale. So this problem was likely introduced then.

pixelzoom commented 6 years ago

This is due to problems inherent in representing floating point (decimal) numbers in JavaScript.

Example: Green candy is $8.20/lb. One bag of candy is 0.3lb. And $8.20 0.3 = $2.46. But not in JavaScript land, where $8.20 0.3 = 2.4599999999999995. So the scale would be showing 2.459, with the '9' grayed out. But in the Shopping screen, this isn't supposed to happen, so the 3rd decimal place is hidden, resulting in the scale showing $2.45.

pixelzoom commented 6 years ago

Fixed in 1.1.0-dev.1.

@arouinfar please verify. Look very carefully at both the Shopping and Shopping Lab screens, because this affected the cost display for both screens. The Shopping screen should now do nearest-neighbor rounding to 2 decimal places (like the DNL). The 3rd (gray) decimal place on the Shopping Lab screen should continue to behave as specified.

If everything looks OK, assign back to me and I'll do a maintenance release in the 1.0 branch.

arouinfar commented 6 years ago

Thanks @pixelzoom!

I used the following method on the Shopping screen to test all 12 items, and everything looked good.

  1. Place one bag on the scale
  2. Compare cost on scale to DNL
  3. Repeat (1) and (2) for the rest of the bags
  4. For fruits, remove items one-by-one and compare the cost on the scale to the DNL
  5. Repeat (1)-(4) to double check

I also tested a few sets of Questions -- perhaps one or two sets per scene. I have not exhaustively tested the Questions since I believe those are separate from the scale rounding issue. @pixelzoom please let me know if you think if further testing is necessary.

On Shopping Lab, I followed the method described in https://github.com/phetsims/unit-rates/issues/44#issuecomment-282110931 to test each scene for a few different denominators (listed below). I went a bit further than the linked procedure, and tested all bag configurations for each scene.

Things were looking good, and then I ran into several scenarios where the rate divides evenly, but the cost on the scale appears to encounter a floating point error.

Item Rate Price on Scale (incorrect) Price on DNL (correct)
3 bags carrots $3/20 $1.79 $1.80
3 bags carrots $6/20 $3.59 $3.60
3 bags carrots $7/20 $4.199 $4.20
3 bags carrots $12/20 $7.19 $7.20
3 bags carrots $14/20 $8.399 $8.40
3 bags carrots $19/20 $11.399 $11.40
1 bag purple candy $7/20 lb $0.139 $0.14
1 bag purple candy $14/20 lb $0.279 $0.28
2 bags purple candy $7/20 lb $0.279 $0.28
2 bags purple candy $14/20 lb $0.559 $0.56
4 bags purple candy $7/20 lb $0.559 $0.56
4 bags purple candy $14/20 lb $1.119 $1.12

I compared these cases to dev.60 and dev.59. Unfortunately, this matches the behavior of dev.60, so original testing was not exhaustive enough. It looks like dev.59 had some errant 0's in the third decimal place on the scale, but was otherwise correct (eg. $0.56 displayed as $0.560).

pixelzoom commented 6 years ago

Let's look at the first row in the table above:

Item Rate Price on Scale (incorrect) Price on DNL (correct
3 bags carrots $3/20 $1.79 $1.80

There are 4 carrots per bag, so 3 bags is 12 carrots. With a calculator, $3 / 20 12 = $1.80. With JavaScript, 3 / 20 12 = 1.7999999999999998. And I have no way of knowing whether this is a valid computation or floating point error. Not sure how I'm going to resolve this one.

pixelzoom commented 6 years ago

1.1.0-dev.2 changes the algorithm used to determine whether the cost displayed on the scale has a meaningful 3rd decimal place. It first rounds the cost to 10 decimal places, in an attempt to remove floating point error. This will work only if we don't have any results that are supposed to be numbers like 1.7999999999999998.

I tested with the scenarios reported in https://github.com/phetsims/unit-rates/issues/202#issuecomment-335573728 and https://github.com/phetsims/unit-rates/issues/202#issuecomment-335869763, and results looked good.

@arouinfar please verify. And again, please look very carefully at both the Shopping and Shopping Lab screens.

arouinfar commented 6 years ago

Looks great @pixelzoom! Please proceed with the maintenance release.

I followed the same general procedure as in https://github.com/phetsims/unit-rates/issues/202#issuecomment-335869763, but tested several additional denominators on the Shopping Lab screen which would lead to costs that terminate at the hundredths place. I did not encounter any floating point errors.

arouinfar commented 6 years ago

@pixelzoom once the maintenance release has been published, please assign @oliver-phet to notify the user.

pixelzoom commented 6 years ago

@arouinfar Before I proceed... Did you also verify that Shopping Lab scenarios that should have a 3rd decimal place still do? Since the fix involves an additional rounding (vs truncating) phase, I want to make sure that we didn't lose any 3rd decimal places.

arouinfar commented 6 years ago

@pixelzoom I also tested many, many cases with the 3rd decimal place, and the truncation on the scale and rounding on the DNL appear to be working as intended.

pixelzoom commented 6 years ago

Excellent, thanks @arouinfar. I will proceed with a maintenance release.

pixelzoom commented 6 years ago

This fix has been deployed in version 1.0.4, now available on the PhET website. @oliver-phet please notify and thank the user.

oliver-phet commented 6 years ago

Email to user sent. Closing.