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

round(num,2) not rounding #5

Closed
getodk-bot opened this issue Nov 23, 2016 · 6 comments
Closed

round(num,2) not rounding #5

getodk-bot opened this issue Nov 23, 2016 · 6 comments
Assignees
Labels

Comments

@getodk-bot
Copy link
Member

Issue by mitchellsundt
Thursday Jul 09, 2015 at 19:41 GMT
Originally opened as getodk/getodk#1030 (0 comment(s))


Originally reported on Google Code with ID 1029

round('14.29123456789',0) = 14
round('14.29123456789',1) = 14.3
round('14.29123456789',2) = 14.290000000000001
round('14.29123456789',3) = 14.291
round('14.29123456789',4) = 14.2912

Reported by yanokwa@nafundi.com on 2014-07-02 17:14:41


- _Attachment: [Basic.xml](https://storage.googleapis.com/google-code-attachments/opendatakit/issue-1029/comment-0/Basic.xml)_
@dcbriccetti
Copy link
Contributor

I added a test for this to XPathEvalTest, and modified the testing mechanism to no longer silently hide errors caused by unrepresentable floating point numbers, and we now see this:

⋮
Running org.javarosa.xpath.test.XPathEvalTest test: math operators
6.1 - 7.8 result -1.7000000000000002 differed from expected -1.7
1.1 * -1.1 result -1.2100000000000002 differed from expected -1.21
Running org.javarosa.xpath.test.XPathEvalTest test: math functions
round('14.29123456789', 2) result 14.290000000000001 differed from expected 14.29
⋮

The results of the first two, the subtraction and the multiplication, aren’t representable in floating point, as you can see here:

scala
Welcome to Scala 2.12.1 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_77).
Type in expressions for evaluation. Or try :help.

scala> 6.1d - 7.8d
res0: Double = -1.7000000000000002

scala> 1.1d * -1.1d
res1: Double = -1.2100000000000002

However, the expected result of the round operation, 14.29, is representable. The error is introduced by JavaRosa’s rounding algorithm round, in XPathFuncExpr, which at one point repeatedly divides by 10.0:

while ( p < 0 ) {
  num /= 10.0;
  ++p;
}

giving these values:

1429.0
142.9
14.290000000000001

There’s the explanation. Now what shall we do about it? I’m inclined to consider an approach like this:

scala> val nf = java.text.NumberFormat.getInstance()
nf: java.text.NumberFormat = java.text.DecimalFormat@674dc

scala> val num = 14.29123456789
num: Double = 14.29123456789

scala> nf.setMaximumFractionDigits(2)

scala> val str = nf.format(num)
str: String = 14.29

scala> val result = str.toDouble
result: Double = 14.29

I wonder why JavaRosa’s round function doesn’t do it this way. Performance? NumberFormat not being available on Android?

@lognaturel
Copy link
Member

I can't see any obvious reasons it wouldn't have been done that way. It looks like all the required functionality has been in Java since the beginning of Java. It could have maybe been performance in the J2ME days but I can't imagine that'd still be a problem. @mitchellsundt @yanokwa any reason not to use NumberFormat for rounding?

@yanokwa
Copy link
Member

yanokwa commented Apr 20, 2017

I don't know offhand.

For what it's worth, here's what CommCare does...
return new Double(Math.floor(FunctionUtils.toDouble(evaluatedArgs[0]) + 0.5));

Source: https://github.com/dimagi/commcare-core/blob/master/src/main/java/org/javarosa/xpath/expr/XPathRoundFunc.java

@dcbriccetti
Copy link
Contributor

dcbriccetti commented Apr 24, 2017

CommCare’s round takes just one argument. Ours takes 2.

        testEval("round(1.5)", null, null, new Double(2.0));
        testEval("round(-1.5)", null, null, new Double(-1.0));

versus

        testEval("round('14.29123456789', 0)", null, null, 14.0);
        testEval("round('14.29123456789', 1)", null, null, 14.3);

One of our code comments: // non-standard Excel-style round(value,decimal place)

@dcbriccetti
Copy link
Contributor

I have implemented a solution. See 7d0e596.

lognaturel added a commit that referenced this issue May 3, 2017
lognaturel added a commit that referenced this issue May 3, 2017
@lognaturel
Copy link
Member

Closed in #33

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

No branches or pull requests

5 participants