Calculator does not work properly with operations with scientific notation numbers

RESOLVED FIXED in B2G C3 (12dec-1jan)

Status

Boot2Gecko Graveyard
Gaia::Calculator
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: willyaranda, Assigned: baku)

Tracking

unspecified
B2G C3 (12dec-1jan)
x86
Mac OS X

Details

Attachments

(4 attachments, 2 obsolete attachments)

Created attachment 689456 [details]
screenshot #1

For example, the number 1.4322661567719377e+25 (you can get a big number by multiplying big numbers) (screenshot #1), multiplied by 9 (screenshot #2), gives 226.432 (screenshot #3), which is totally wrong.
blocking-basecamp: --- → ?
This app isn't a Calculator for scientists
blocking-basecamp: ? → -
And the result is correct.
What it's doing is 25*9 then adding +1.432  the result is correct in that e is not being counted.
Oh!  My mistake.  I see what you mean by getting the large number.  I thought the e was being typed.  The value after getting an e is incorrect...  This is kinda bad, we also end up rounding up at the 3 decimal place.

clee, if you think this is bad please renom.
Flags: needinfo?(clee)

Comment 7

5 years ago
(In reply to dscravaglieri from comment #4)
> And the result is correct.

the result should be wrong.

4.320e+3 is equal to 4320.
in this case, the result will be 1.4322661567719377e+25 (due to the number of 9 is too small to the number, it will be ignored)
(Assignee)

Comment 8

5 years ago
Created attachment 691870 [details] [diff] [review]
patch

Here I propose a better parser for numbers and operators. Give me a feedback :)
Attachment #691870 - Flags: review?(dale)
(Assignee)

Comment 9

5 years ago
Created attachment 692079 [details] [diff] [review]
patch b

I just moved my code in a separated file. It's easier to debug it.
Attachment #691870 - Attachment is obsolete: true
Attachment #691870 - Flags: review?(dale)
Attachment #692079 - Flags: feedback?(dale)
Hey, I think this is definitely important to support. I was actually working on another bug, and had to fix this for the end solution.

My open pull request is located here: https://github.com/mozilla-b2g/gaia/pull/6934

Essentially I'm just converting the numbers to a string before the evaluation, whereas you are tokenizing. The end solution doesn't really matter to me, I do really just want to see this fixed.

Comment 11

5 years ago
(In reply to Al Tsai [:atsai] from comment #7)
> (In reply to dscravaglieri from comment #4)
> > And the result is correct.
> 
> the result should be wrong.
> 
> 4.320e+3 is equal to 4320.
> in this case, the result will be 1.4322661567719377e+25 (due to the number
> of 9 is too small to the number, it will be ignored)

Damn it. I found that is a multiply calculation, and the result should be something like 1.2890394...e+26
Current result is definitely a wrong one.
(Assignee)

Comment 12

5 years ago
> My open pull request is located here:
> https://github.com/mozilla-b2g/gaia/pull/6934

I think we should have this code reviewed before.
Take a look of my patch, maybe we can merge the 2 approaches.
I implemented a 'full' math expression parser, a float number parser etc.

Comment 13

5 years ago
Calculator is now a v1 core app so we need this fixed.  Let's mark blocking.  (We can't have the results of the calculator wrong, that's not an option.)
blocking-basecamp: - → ?
Flags: needinfo?(clee)
blocking-basecamp: ? → +
Priority: -- → P2
(Assignee)

Updated

5 years ago
Attachment #692079 - Flags: feedback?(dale) → review?(dale)
Assignee: nobody → amarchesini

Updated

5 years ago
Target Milestone: --- → B2G C3 (12dec-1jan)
Comment on attachment 692079 [details] [diff] [review]
patch b

Very sorry for dropping the ball in this review, I thought I was up to date until I checked 'My Requests' today

The implementation of the number parser is cleaner in my opinion, however this fix will not fix the precision of the result bug that kevin's patch does.

Andrea would you mind integrating Kevins tests into your patch (which will require the formatNumber fix as well)

I will clear the review request until we have the extra tests in place, however the code looks good and is working good for me here
Attachment #692079 - Flags: review?(dale)
(Assignee)

Comment 15

5 years ago
Created attachment 693423 [details] [diff] [review]
patch c

Tested added.
Attachment #692079 - Attachment is obsolete: true
Attachment #693423 - Flags: review?(dale)
Renom - bug 822565 indicates calculator app is no longer in v1.
blocking-basecamp: + → ?
Attachment #693423 - Flags: review?(dale) → review+
Ah, sorry this got renomed right in the middle of me reviewing + merging

Merged in: https://github.com/daleharvey/gaia/commit/77a7a987566a6834307b981ce78c15943c4f1615
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
blocking-basecamp: ? → +
Calculator app is not part of Gaia anymore.
Status: RESOLVED → VERIFIED
Resolution: FIXED → INVALID
For v1.  It's still a bug for future that we should check if it does get readded.  As far as I understand we are adding it back in.  removing it from invalid.
Status: VERIFIED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: INVALID → FIXED
Component: Gaia::Calculator → Gaia::Calculator
Product: Boot2Gecko → Boot2Gecko Graveyard
You need to log in before you can comment on or make changes to this bug.