Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User reported issue: Green candy price #202

Closed
oliver-phet opened this issue Oct 9, 2017 · 16 comments
Closed

User reported issue: Green candy price #202

oliver-phet opened this issue Oct 9, 2017 · 16 comments

Comments

@oliver-phet
Copy link

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
Copy link

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
Copy link

arouinfar commented Oct 10, 2017

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

pixelzoom commented Oct 11, 2017

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.

@pixelzoom pixelzoom removed their assignment Oct 11, 2017
@arouinfar
Copy link

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 #44 (comment) 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.

  • 9: Most costs will have a repeating decimal
  • 17: Most costs will look irrational
  • 20: All costs will be exactly divisible, so the 3rd digit should never be visible.

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).

@arouinfar arouinfar assigned pixelzoom and unassigned arouinfar Oct 11, 2017
@pixelzoom
Copy link
Contributor

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
Copy link
Contributor

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 #202 (comment) and #202 (comment), and results looked good.

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

@arouinfar
Copy link

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

I followed the same general procedure as in #202 (comment), 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 arouinfar assigned pixelzoom and unassigned pixelzoom and arouinfar Oct 12, 2017
@arouinfar
Copy link

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

@pixelzoom
Copy link
Contributor

@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.

@pixelzoom pixelzoom removed their assignment Oct 13, 2017
@arouinfar
Copy link

@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.

@arouinfar arouinfar assigned pixelzoom and unassigned arouinfar Oct 13, 2017
@pixelzoom
Copy link
Contributor

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

pixelzoom added a commit that referenced this issue Oct 14, 2017
pixelzoom added a commit that referenced this issue Oct 14, 2017
pixelzoom added a commit that referenced this issue Oct 14, 2017
@pixelzoom
Copy link
Contributor

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

@pixelzoom pixelzoom assigned oliver-phet and unassigned pixelzoom Oct 15, 2017
@oliver-phet
Copy link
Author

Email to user sent. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants