After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 485142 - Calculation looses accuracy if the user has been "hand editing" the display.
Calculation looses accuracy if the user has been "hand editing" the display.
Status: RESOLVED FIXED
Product: gnome-calculator
Classification: Core
Component: general
5.21.x
Other All
: Normal normal
: ---
Assigned To: Sami Pietilä
Rich Burridge
Depends on:
Blocks: 500994
 
 
Reported: 2007-10-09 17:50 UTC by Rich Burridge
Modified: 2010-04-15 02:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Potential patch from Sami to fix the problem. (9.01 KB, patch)
2007-10-09 17:51 UTC, Rich Burridge
none Details | Review
Patch from Sami to regain stability with aritmetic precedence. (1.56 KB, patch)
2007-10-23 20:49 UTC, Rich Burridge
committed Details | Review
Expands tokens (Ans, R0-R9) only when required (7.97 KB, patch)
2007-12-23 06:17 UTC, Robert Ancell
needs-work Details | Review

Description Rich Burridge 2007-10-09 17:50:14 UTC
From the work done to fix bug #326938, it's been discovered
that the display will loose accuracy in arithmetic precedence
mode if the user has been "hand editing" the display.
Comment 1 Rich Burridge 2007-10-09 17:51:14 UTC
Created attachment 96953 [details] [review]
Potential patch from Sami to fix the problem.
Comment 2 Rich Burridge 2007-10-09 19:24:13 UTC
Just tried the patch. I did the following with gcalctool
in arithmetic precedence mode with initial accuracy of 4
significant places (i.e. "0.0000").

First experiment:

1/ Enter 1 / 7 <Return>
   "0.1429.." is displayed.
2/ Press the Acc button and tried to select 9 significant places.
   Nothing happened. Stayed at 4 significant places.


Second experiment:

1/ Enter 1 / 9 <Return>
   "0.1111.." is displayed
2/ Enter * 9 <Return>
   "1.0000.." is displayed. Should that just be "1.0000" now?

Comment 3 Sami Pietilä 2007-10-09 19:40:40 UTC
>1/ Enter 1 / 7 <Return>
>   "0.1429.." is displayed.
>2/ Press the Acc button and tried to select 9 significant places.
>   Nothing happened. Stayed at 4 significant places.

I think there is nothing yet in ACC-button callback that updates this. However, pressing enter two times might update this value.

>1/ Enter 1 / 9 <Return>
>   "0.1111.." is displayed
>2/ Enter * 9 <Return>
>   "1.0000.." is displayed. Should that just be "1.0000" now?

I think this is caused by the same reason than following problem (in my gcalctool 5.9.14, in ubuntu feisty)

Write "1/3"
Press Enter
Write "*3"
Press Enter
Write "OR 1"
Press Enter.

Gcalctool seems to think that the first "1" is not an integer.
Comment 4 Sami Pietilä 2007-10-13 14:38:07 UTC
>Write "1/3"
>Press Enter
>Write "*3"
>Press Enter

Rich, can you check why MP library thinks in this case that the result is not equal to one? The MP library also thinks that the result is not an integer, however the display is showing "1".
Comment 5 Rich Burridge 2007-10-13 15:34:36 UTC
I don't think there is anything wrong with the MP library.
This works in left-to-right precedence mode. In Scientific 
mode, with 9 significant places I get:

>Write "1/3"
>Press Enter

Display shows 0.333333333

>Write "*3"
>Press Enter

Display shows 1

I know that the arithmetic precedence mode goes through a
different code path, so I suggest comparing the two paths
and see how they differ.

My guess is that the left-to-right precedence mode is always
working off an MP number (the previous result) after you've 
pressed Enter, whereas the arithmetic precedence mode is starting 
from what is shown in the actual calculator display. So it's 
using "0.333333333" and multiplying that by 3 and getting "0.999999999".

Perhaps the best thing here (and maybe this helps fix bug #347630)
is for the arithmetic precedence mode to have an off-screen
area which it can mark up with notation to indicate different things.
When the user presses Return, the result is calculated from that.
For example, where the display might have "0.333333333", the off-screen
area would have "[RES]", to indicate that the last MP result should 
be used. For bug #347630, where the display would have the two unicode
characters, the off-screen area would have the "normal" arithmetic
operators.

This approach would save having to show things like "1.0000.." in the
display, which is only going to confuse the user.
Comment 6 Sami Pietilä 2007-10-13 16:48:08 UTC
Please apply the patch in this pr before running previous test case.

However, let me use another test-case, which is:

write:
 "1/3*3" 
then press enter.

This is calculated with full precision using MP library functions.

Now adding a debug function that tells if calculated result is an integer or not says that the result is not an integer, while the display is showing 1. This is why "(1/3*3) OR 1" fails. In arithmetic precedence mode there is a check that bitwise operations can not be done with non-integers (=decimal numbers).





Comment 7 Rich Burridge 2007-10-13 21:05:04 UTC
> In arithmetic precedence mode there is a check that bitwise operations 
> can not be done with non-integers (=decimal numbers).

And in left-to-right precedence mode there isn't. Perhaps the simplest
fix here is to code the arithmetic precedence mode the same way as
left-to-right precedence.

Or maybe always do the the MP equivalent of int(n) before you do the 
logical operations.

I notice that "Int(1/3*3) OR 1" works just fine in arithmetic precedence
mode.
Comment 8 Sami Pietilä 2007-10-14 07:14:27 UTC
>I notice that "Int(1/3*3) OR 1" works just fine in arithmetic precedence
>mode.

Yes, this should be mathematically equal to "1/3*3 OR 1". Unfortunately now mp library thinks it is not.

>> In arithmetic precedence mode there is a check that bitwise operations 
>> can not be done with non-integers (=decimal numbers).

>And in left-to-right precedence mode there isn't. Perhaps the simplest
>fix here is to code the arithmetic precedence mode the same way as
>left-to-right precedence.

Unfortunately, this mp library failure causes various other problems too. I think the real cause must be fixed.

If this mp problem is hard to fix, then we should try to write is_integer() and is_natural() functions, which would work despite of this mp problem.
Comment 9 Rich Burridge 2007-10-14 14:03:21 UTC
Okay, understood. I suspect the problem is in the code that was
added (unfortunately not by me -- I would have put it in functions.c)
at about line 1195 in mp.c, specifically to handle this problem. 

Recently, in order to fix bug #439087, the maximum number of displayable
digits was increased to 200 and the size of MP numbers internally (MP_SIZE)
was adjusted to 1000.

I suspect the chunk of code:

...
    v->dtype = FIX;
    v->accuracy = MAX_DIGITS;
    mpcmf(&x[1], tmp);
    STRCPY(disp, make_number(tmp, v->base, FALSE));

    if (disp[0] == '1') {
        y[ll+1]++;
    }
...

is no longer doing the right thing.

One simple test is to reinstate the old values:

#define MP_SIZE      150     /* Size of the multiple precision values. */
#define MAX_DIGITS     40    /* Maximum displayable number of digits. */

and see if it starts working for you.

Could you check that out please?
Comment 10 Sami Pietilä 2007-10-14 15:17:25 UTC
>#define MP_SIZE      150     /* Size of the multiple precision values. */
>#define MAX_DIGITS     40    /* Maximum displayable number of digits. */

Unfortunately, changing the defines did not seem to have any effect on the bug.
Comment 11 Rich Burridge 2007-10-14 15:31:03 UTC
Did this bug exist in gcalctool in GNOME 2.20? 

If so, then the recent changes to increase the number of 
displayable digits have not had any adverse effect on this,
and that was a red herring.

As mentioned to you in email, you already have is_integer() 
and is_natural() routines in mpmath.c, so it looks like the 
fix is going to be in the mpcmim routine in mp.c (as 
mentioned in a previous comment), which is called by is_integer().

Comment 12 Rich Burridge 2007-10-23 20:49:11 UTC
Created attachment 97754 [details] [review]
Patch from Sami to regain stability with aritmetic precedence.

Patch committed.
Comment 13 Sami Pietilä 2007-11-22 14:19:43 UTC
This pr is obsolete because the original hand-edit does not exist anymore in SVN trunk.
Comment 14 Robert Ancell 2007-11-22 23:41:01 UTC
Just a note about the current behaviour:

The calculator has two strings, the equation being solved and the display. The display is made by substituting the 'Ans' and 'R0'-'R9' symbols with MP register values.

e.g. Typing '1/7=' sets the strings to:
calculation = 'Ans'
display = '0.14' (depending on the precision)

If the user types '+' with the cursor at the end of the display (the default location after '=' is pressed) then the calculation is appended, i.e.:
calculation = 'Ans+'
display = '0.14+'

If the cursor was left one character the calculation is generated from the display and precision is lost, i.e:
calculation = '0.1+4'
display = '0.1+4'

Seems like a good compromise, however precision can be unnecessarily lost, e.g.:
If we have the following situation:
calculation = 'Ans*3-6'
display = '0.14*3-6'
and the user wants to put brackets around the '3-6' we will loose precision as the logic is 'use the display as the calculation if the cursor is not at the end of the display'. The following occurs:
calculation = '0.14*(3-6)'
display = '0.14*(3-6)'

In the previous case we could have been able to keep the precision of the 1/7 but we have lost it as we don't know if the cursor is inside the 'Ans' variable. To fix this the display code needs to be a lot smarter.

In conclusion: we can lose precision unnecessarily when editing the display but it is less severe than before.
Comment 15 Sami Pietilä 2007-11-23 09:21:26 UTC
Hi, 

I did not realize that the pr is also valid for new hand-edit feature. Re-opening this pr.

The idea behind previously released versions (without hand-edit) was that accuracy is not lost unless user tries to edit the answer (ANS) itself.

I just tried the SVN version and it is like Robert said, that accuracy is lost in some cases even if user has not been edited the ANS. This is regression from previous version and basically means that in some cases the calculator calculates wrongly.

Please take a look at the previous hand-edit version (http://users.utu.fi/sampie/gcalctool/gcalctool-with-hand-edit.tar.gz). It allows full editing and does not suffer from previously described problem. It however, has a bug which identifies all numbers with ".." marking as ANS. I hope to fix that one soon.
Comment 16 Sami Pietilä 2007-11-25 11:44:43 UTC
Here is a version with some wrinkles ironed out. Please still use only keyboard for editing the display. However, this technique allows adding a proper button support.

This is only a proof-of-concept and the code-base is way older than SVN trunk.

http://users.utu.fi/sampie/gcalctool/gcalctool-with-hand-edit2.tar.gz
Comment 17 Robert Ancell 2007-11-25 12:56:36 UTC
I checked out revision 1759 which had the previous display editing and can confirm there has not been a regression and the behaviour is better:

Example A (use buttons only):
1. Open calculator and set accuracy to 1sf
2. Type "1/7="
3. Type "+1="
4. Type "-1="
5. Type "*7="

Example B (hand edit):
1. Open calculator and set accuracy to 1sf
2. Type "1/7="
3. Click in the display to the right
4. Type "+1="
5. Type "-1="
6. Type "*7="

On the subversion head both example A and B give the result "1". In revision 1759 example A gives the result "1" and example B "0.7".

The difference is revision 1759 loses accuracy as soon as the display is clicked in where the head loses precision if the display is not appended (i.e. inserting/deleting characters).

Sami can you explain how the attached versions retain accuracy as it is not clear from looking at the source how this occurs.

It appears to me when enter is pressed:
1. The display is copied from the GtkTextEntry using get_expr_from_display()
2. This is set to the current expression using exp_replace()
3. The display is processed as normal using do_expression()

Which appears to me to use the displayed value not the high accuracy value.
Comment 18 Sami Pietilä 2007-11-25 13:26:34 UTC
>I just tried the SVN version and it is like Robert said, that accuracy is lost
>in some cases even if user has not been edited the ANS. This is regression from
>previous version and basically means that in some cases the calculator
>calculates wrongly.

Ah, this was a little bit misleading. Sorry. I meant that this is regression from officially released version (which did not have hand-edit).

>Sami can you explain how the attached versions retain accuracy as it is not
>clear from looking at the source how this occurs. 

Actually, the whole hidden buffer is removed. The expression text can be edited as a such. If an answer is trunkated it is marked with two dots. This enables parser to detect it and use full-accuracy value.
Comment 19 Rich Burridge 2007-11-25 16:29:51 UTC
> Actually, the whole hidden buffer is removed. The expression text can be edited
> as a such. If an answer is truncated it is marked with two dots. This enables
> parser to detect it and use full-accuracy value.

Now that we no longer use that approach, we are going to have to come
up with something that works with the current model. I suggest trying
to understand what Robert has created and work from there. Thanks.


Comment 20 Robert Ancell 2007-11-25 22:23:52 UTC
Ah I see, I saw the two dots were being added but couldn't see where they were being processed. Does this raise the possibility of the parser getting confused between the Ans and R0 substitutions?

e.g. if Ans is '0.123456' and R0 is '0.111111' and the accuracy is set to 1sf the expansion of Ans+R0 will be:
0.1..+0.1..

Will the parser correctly solve this as 0.123456+0.111111? Or 0.123456+0.123456?

Perhaps there is a simpler solution to this. If we do not expand the Ans or Rn variables then the user will never lose accuracy. This is the way that all the physical scientific calculators I have used work. I raised this idea indirectly with #493694 which I opened when I didn't understand how gcalctool worked internally.

More exactly the logic is:
The display is set to the calculation unless the calculation is 'Ans'/'R0'/.../'R9'. In this case the display is replaced with the numeric value rendered in the selected base/accuracy. Any editing of this rendered number will cause accuracy to be lost.
Comment 21 Sami Pietilä 2007-11-26 12:38:45 UTC
>Ah I see, I saw the two dots were being added but couldn't see where they were
>being processed. Does this raise the possibility of the parser getting confused
>between the Ans and R0 substitutions?

The two dots are processed in ce_parser and in ce_tokenizer. Using two dots or any other marking enables much simpler approach, which avoids a lot of problems related to the difficulty of handling the hidden buffer.

The current idea support only answer expanding at this point. Registers can be displayed as symbols Rn, where n is a register number. This is how gcalctool used to work for a long time.

(Personally, I would prefer also to expand registers if it is possible. But if registers can not be expanded I think it is acceptable.)

>Perhaps there is a simpler solution to this. If we do not expand the Ans or Rn
>variables then the user will never lose accuracy. This is the way that all the
>physical scientific calculators I have used work. I raised this idea indirectly
>with #493694 which I opened when I didn't understand how gcalctool worked
>internally.

I was thinking the same. However, I think we can not give up answer expanding. That is how many users expect calculator work. Gcalctool is a hyprid of expression calculator and traditional calculator. It relies heavily on the use-cases of traditional calculators in which answer is shown using numbers.

>More exactly the logic is:
>The display is set to the calculation unless the calculation is
>'Ans'/'R0'/.../'R9'. In this case the display is replaced with the numeric
>value rendered in the selected base/accuracy. Any editing of this rendered
>number will cause accuracy to be lost.

Yes, this is how it should work. This has turned out to be a quite good approach.

Comment 22 Robert Ancell 2007-12-23 06:17:17 UTC
Created attachment 101490 [details] [review]
Expands tokens (Ans, R0-R9) only when required

This patch adds an exp_expand() which expands tokens (Ans, R0-R9) in e->expression if the cursor is inside one. Unfortunately the algorithm is very complex...

What still needs to be solved is what happens after enter is pressed. The current behaviour is if the equation is solved if the next key is a digit the display is replaced with the new number. e.g.:

Type 1234 (enter). The display shows '1234'. Type 1; the display shows '1'.

Now the display can be edited at any point this behaviour doesn't seem quite right, e.g. if you want to prefix the above '1234' with '1+' the user would:

Type 1234 (enter). The display shows '1234'. Press home to get the cursor before the '1234'. Typing '1+' will replace the display with '1+' instead of prefixing it.

The simplest solution is to never replace the display, instead leave it for the user to clear (shift-delete).