Fix significant figures and round off for trigonometric values

[nswarup14]

Fixes #48

Changes

1. functions.py

Updates

1. Round off trigonometric results to a maximum of 6 decimal figures, by calculating the number of significant figures
2. Fix the noise values for equations like cos(90), sin(0).
3. tan(90) returns infinity as the answer rather than an arbitrary large value
4. Works for both types of input, degrees and radians

Tested on Ubuntu 18.04 and Sugar v0.114

[quozl]

Thanks. Reviewed 815959b. Why six digits? A repeating code pattern you have used is counting the number of significant digits, limiting to six, and then rounding. Please refactor this. significant() uses a local variable "integral", which has ambiguous meaning (wiktionary), would "integer" be more appropriate? round() is removed from body of source, but not from function table, and continues to be used in 13 other places, so the built-in function is now used. Commit message doesn't tell me if this was intentional. docstring of xor() was unnecessarily changed. Our documentation, and possibly also teacher lesson plans we don't have access to, still refer to ceil() and round(). Please either leave these functions available, or update https://help.sugarlabs.org/en/calculate.html

[nswarup14]

would "integer" be more appropriate?

Yes, I think so too. I wasn't aware of the ambiguous meaning for "integral".

docstring of xor() was unnecessarily changed.

I have corrected that. Might have slipped me the first time.

round() is removed from body of source, but not from function table,
and continues to be used in 13 other places, so the built-in function
is now used. Commit message doesn't tell me if this was intentional.

Yes, now the built-in function for round is used. Given that the round() and ceil() functions as mentioned in the activity documentation are still used, I have reverted back their removal. However, I have updated the function prototype for round() to round_to_integer() and updated in the function table as well. Is that alright? Do I need to update the documentation as well?

I have updated the round_to_integer() function, as math.round() function does not exist.

A repeating code pattern you have used is counting the number of
significant digits, limiting to six, and then rounding. Please
refactor this.

I have asked this on the issue. Based on consensus, I shall refactor the code

Thanks!

[rhl-bthr]

Tested. Same review comments as @quozl

[rhl-bthr]

Please see inline comments

[nswarup14]

@quozl @pro-panda
As we had discussed, I have updated the way significant figures are calculated in accordance with http://chemistry.bd.psu.edu/jircitano/sigfigs.html.

I have also removed the condition of displaying at most upto 6 decimal figures.
The result is rounded off only to the number of significant digits it possess. I have also included conditions which return 0 in the relevant cases, thus preventing unnecessary noise.

Thanks

[quozl]

Thanks. Tested f01497a.

With number of shown digits set to 6, calculation sin(45) yields 0.707106

With number of shown digits set to 15, calculation sin(45)*100 yields 70.7106781187

With number of shown digits set to 6, calculation sin(45)*100 yields 70.7106; should this be 70.7107?

With number of shown digits set to 6, calculation 123.456+0.0007 yields 123.456; should this be 123.457?

I'm having trouble assessing this pull request because the target has changed along the way, and so the commit messages are out of date. Could you write a new commit message please? Either as a comment marked as a proposed commit message, or if you want to do it the git way; revert all five commits in reverse order, then make a new commit with the changes and the complete commit message. Let me know if you need more detail on that procedure.

[nswarup14]

@quozl

With number of shown digits set to 15, calculation sin(45)*100 yields 70.7106781187.

Yes, this is because the calculated result itself is 70.7106781187. Hence the displayed result is also the same.

With number of shown digits set to 6, calculation sin(45)*100 yields 70.7106; should this be 70.7107?

No, because the the result calculated is 70.7106781187, the displayed result is just the first 6 digits i.e 70.7106. The result is rounded off only when it is computed. However, the display result is computed is differently.

With number of shown digits set to 6, calculation 123.456+0.0007 yields 123.456; should this be 123.457?

No. This is because 123.456 + 0.0007 yields 123.4567 and the truncated result is 123.456. The reason being same above.

The present patch hopes to correct the noise introduced in trigonometric calculations by computing the number of significant figures and then rounding off. However, the displayed result is computed differently. The rounding off is not a part of truncating the final display result, hence 123.4567, even with the number of shown digits as 6, is displayed as 123.456 and not 123.457.

[rhl-bthr]

Swarup and I had a call. Summary:
Instead of using significant figures at the time the result is calculated and truncating before it is displayed, we should round off the result before it is displayed.

[nswarup14]

@quozl @pro-panda
As per discussion, I have reverted the earlier commits in the reverse order and worked from there onwards.

Rather than modifying the result during calculation, the result is rounded off just before display. The following evaluates the user experience.

• Based on the number of shown digits parameter, the displayed result is rounded off to the right number of decimal places.
• If the value lies between (-1, 1), then the value is directly rounded off.
• If the value is otherwise, then the length integer part of the result is calculated and the result is rounded to the remaining decimal places.
• If the length of the integer part of the result is greater than the number of digits shown parameter, the result is displayed unchanged/unmodified.

eg: With number of shown digits set to 6, calculation sin(45) yields 0.707106
eg: With number of shown digits set to 15, calculation sin(45)*100 yields 70.7106781187
eg: With number of shown digits set to 6, calculation sin(45)*100 yields 70.7107
eg: With number of shown digits set to 6, calculation 123.456+0.0007 yields 123.457

Thanks!

[rhl-bthr]

Thanks. Reviewed, did some testing.

Please do more testing to ensure that this change does not break anything else

[quozl]

Thanks. Tested 7565cf7 on Ubuntu 19.04. Results seem okay. Wish we had a comprehensive test case to run. What do you plan for FIXME: Should this case be handled differently?

[nswarup14]

@quozl
For the FIXME, it deals with numbers whose integer parts is longer than the number of shown digits property.

For example, if 1234.5678 was the answer to a problem and the number of shown digits property was set to 3, we cannot round off the answer to 1230, because that would be wrong.

At the moment, no manipulation of the result takes place for such cases. I propose that, we can perhaps round of the figure to a certain number of decimal places (which will be a predefined number) without affecting the integer part of the answer.

We could even display the result with no manipulation, as it is being done now

Thanks

[quozl]

I'm not sure I understand. In the example you give, 1234.5678 should be shown as 1234.568 when number of shown digits is three? At the moment it is shown as 1234.5678.

[nswarup14]

@quozl
Yes, result 1234.5678, with number of shown digits set to 3, is displayed as 1234.5678. The property 3, is the total number of digits in the answer, excluding trailing or leading zeroes, and not the number of decimal places. Hence 1234.5678 is not shown as 1234.568

What I meant in the FIXME: is that, in cases when the length of the integer part of the answer exceeds the property set, which in this case is 3, we can either leave the answer as it is, or round it off.

I wasn't sure what would be the ideal user behaviour in such cases, hence I added a comment to discuss it later.

Thanks!

[quozl]

Thanks. I'm still a little lost. Commit message for 7565cf7 is much improved, and I can see what you plan for how it is to work, but can you explain in the commit message how it used to work?

[nswarup14]

Yes, I have updated the commit message with more test cases from 475ea8b.

Interestingly, after I do the round off operation in mathlib.py and return str(res), the type cast reduces the number of digits in the result.
For example: str(0.707106781186547) yields 0.707106781187. I looked up the documentation for str(), for converting float to string, and the function does not manipulate the value.

I'm yet to find a cause for this unexpected behavior.

Thanks

[quozl]

Look at the Python source code. It is more accurate than the documentation.

My guess is that the number is converted into a shorter float type prior to formatting as decimal.

You can use repr() instead of str() in this instance. But it would be best to know why str() does not do what you need.

[nswarup14]

@quozl
Thanks!

I replaced the str() to repr() and the conversion to a shorter float type no longer occurs. I have updated the test cases in the commit messages as well.

[quozl]

Please find out why str() does this and repr() does not?

[rhl-bthr]

Also, please resolve conflicts 😺

[nswarup14]

@pro-panda
I have removed the conflicts, but I'm unable to remove the merge commit. Kindly guide me on this.
Thanks.

[rhl-bthr]

Taken from Python 2 docs,

class str(object='')
Return a string containing a nicely printable representation of an object. For strings, this returns the string itself. The difference with repr(object) is that str(object) does not always attempt to return a string that is acceptable to eval(); its goal is to return a printable string.

FYI, this behavior does not occur in Python 3 str()

[rhl-bthr]

Thanks, reviewed

[quozl]

Looking at the overall change relative to master, you are still adding a FIXME comment. I take it you're not yet finished?

[nswarup14]

@quozl
I shall remove it immediately.
As we discussed in the comments (here and here), I changed the FIXME case to round off results to the number of shown digits and display the rounded result.
Thanks.

[quozl]

Thanks. Reviewed.

There are some unnecessary parentheses added if(, and around math.pi. Some of the new lines don't pass flake8.

[nswarup14]

@quozl
I have removed the parenthesis and fixed all flake errors from the lines that were added. Please review
Thanks.

[quozl]

Thanks. Reviewed 1d9259e.

• in removing parentheses from math.pi / 2 you have changed the math, here;

@@ -206,7 +206,7 @@ ceil.__doc__ = _('ceil(x), return the smallest integer larger than x.')
 
 
 def cos(x):
-    if(_scale_angle(x) % (math.pi/2) == 0 and _scale_angle(x) % (math.pi) != 0):
+    if _scale_angle(x) % math.pi / 2 == 0 and _scale_angle(x) % math.pi != 0:
         return 0
     return math.cos(_scale_angle(x))
 cos.__doc__ = _(

and here

@ -510,9 +510,9 @@ sub.__doc__ = _('sub(x, y), return x - y')
 
 
 def tan(x):
-    if(_scale_angle(x) % (math.pi) == 0):
+    if _scale_angle(x) % math.pi == 0:
         return 0
-    if(_scale_angle(x) % (math.pi/2) == 0 and _scale_angle(x) % (math.pi) != 0):
+    if _scale_angle(x) % math.pi / 2 == 0 and _scale_angle(x) % math.pi != 0:
         return "Infinity"
     return math.tan(_scale_angle(x))
 tan.__doc__ = _(

See Python operator precedence.

• if something is evenly divisible by π on two, then it will of course be divisible by π, so the condition can be simplified?

[nswarup14]

@quozl
Thanks for pointing out the mistake with operator precedence. It had slipped my mind. I have corrected that.

if something is evenly divisible by π on two, then it will of course be divisible by π, so the condition can be simplified?

Isn't the vice-versa true? If a number is divisible by, a variable called x, then it will be definitely divisible by (x/2).
I do not see how the condition can be simplified.

Thanks

[quozl]

Let x be 10 instead of math.pi.

Existing code pattern has two conditions and-ed together; here's a truth table;

for n in range(25):
    a = (n % (10 / 2) == 0)
    b = (n % 10 != 0)
    print (n, a, b, a and b)

A simplified code pattern;

for n in range(25):
    print (n, ((n + 5) % 10 == 0))

Same results.

    if (_scale_angle(x) + math.pi / 2) % math.pi == 0:

I've not tested this change, I'm speculating and asking you to comment.

[nswarup14]

@quozl
I understood what you meant by simplifying the if condition.
I tested the code snippets above and got identical results.

Thanks

[quozl]

Tested. When I try "cos(90)" twice, the previous answer acquires a unicode decoration.

[JuiP]

@quozl @chimosky @Saumya-Mishra9129 Without applying the changes in this PR, what does the buttons 3,6,9 do? I tried tan(56) it returned 1.4825609685127403 irrespective of 3,6,9 being selected. Seems like the buttons have no effect on the number of digits displayed.

[quozl]

Without applying the changes in this PR ... the buttons have no effect on the number of digits displayed (by tan()).

Yes, that's the issue. #48