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

fix roundUp and roundDown #43

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
3 participants
@FishMeat

FishMeat commented Dec 5, 2016

roundUp function produces an unexpected floating point approximated value
=ROUNDUP(3987 * 0.2, 2) produces 797.41
but i want get 797.4

fix roundUp and roundDown
roundUp function produces an unexpected floating point approximated value
=ROUNDUP(3987 * 0.2, 2) produces 797.41
but i want get 797.4
@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Dec 8, 2016

Contributor

Would it be possible to add some unit test coverage?

Contributor

pjfanning commented Dec 8, 2016

Would it be possible to add some unit test coverage?

@FishMeat

This comment has been minimized.

Show comment
Hide comment
@FishMeat

FishMeat Dec 9, 2016

@pjfanning

public class POIUnitTest {
    @Test
    public void testFormula() {
        Workbook workbook = new HSSFWorkbook();
        CreationHelper creationHelper = workbook.getCreationHelper();
        FormulaEvaluator formulaEvaluator = creationHelper.createFormulaEvaluator();
        Sheet sheet = workbook.createSheet();
        Row row = sheet.createRow(0);
        Cell cell = row.createCell(0);
        BigDecimal f = new BigDecimal(3987);
        BigDecimal s = new BigDecimal(0.2).setScale(1, BigDecimal.ROUND_HALF_UP);
        String formula = "ROUNDUP(" + f.toString() + "*" + s.toString() + ", 2)";
        cell.setCellFormula(formula);
        double actualValue = formulaEvaluator.evaluate(cell).getNumberValue();
        double expectedValue = f.multiply(s).setScale(2, BigDecimal.ROUND_UP).doubleValue();
        Assert.assertEquals(expectedValue, actualValue, 0.00);
    }
}

FishMeat commented Dec 9, 2016

@pjfanning

public class POIUnitTest {
    @Test
    public void testFormula() {
        Workbook workbook = new HSSFWorkbook();
        CreationHelper creationHelper = workbook.getCreationHelper();
        FormulaEvaluator formulaEvaluator = creationHelper.createFormulaEvaluator();
        Sheet sheet = workbook.createSheet();
        Row row = sheet.createRow(0);
        Cell cell = row.createCell(0);
        BigDecimal f = new BigDecimal(3987);
        BigDecimal s = new BigDecimal(0.2).setScale(1, BigDecimal.ROUND_HALF_UP);
        String formula = "ROUNDUP(" + f.toString() + "*" + s.toString() + ", 2)";
        cell.setCellFormula(formula);
        double actualValue = formulaEvaluator.evaluate(cell).getNumberValue();
        double expectedValue = f.multiply(s).setScale(2, BigDecimal.ROUND_UP).doubleValue();
        Assert.assertEquals(expectedValue, actualValue, 0.00);
    }
}

@FishMeat FishMeat closed this Dec 9, 2016

@FishMeat FishMeat reopened this Dec 9, 2016

@onealj

This comment has been minimized.

Show comment
Hide comment
@onealj

onealj May 16, 2017

Contributor

I tested c144c3d and the following unit test failed on my machine with OpenJDK 1.7.0u65:

public void testRoundDown() {
    double d = 0.049999999999999975d;
    int p = 2;
    assertEquals("round ", 0.04d, MathX.roundDown(d, p));

Here was the build test results when I applied c144c3d.

Testsuite: org.apache.poi.ss.formula.functions.TestMathX
Testcase: testRoundDown took 0 sec
        FAILED
round : Expected 0.04 but was 0.05
junit.framework.AssertionFailedError: round : Expected 0.04 but was 0.05
        at org.apache.poi.ss.formula.functions.AbstractNumericTestCase.assertEquals(AbstractNumericTestCase.java:64)
        at org.apache.poi.ss.formula.functions.AbstractNumericTestCase.assertEquals(AbstractNumericTestCase.java:72)
        at org.apache.poi.ss.formula.functions.TestMathX.testRoundDown(TestMathX.java:754)
Contributor

onealj commented May 16, 2017

I tested c144c3d and the following unit test failed on my machine with OpenJDK 1.7.0u65:

public void testRoundDown() {
    double d = 0.049999999999999975d;
    int p = 2;
    assertEquals("round ", 0.04d, MathX.roundDown(d, p));

Here was the build test results when I applied c144c3d.

Testsuite: org.apache.poi.ss.formula.functions.TestMathX
Testcase: testRoundDown took 0 sec
        FAILED
round : Expected 0.04 but was 0.05
junit.framework.AssertionFailedError: round : Expected 0.04 but was 0.05
        at org.apache.poi.ss.formula.functions.AbstractNumericTestCase.assertEquals(AbstractNumericTestCase.java:64)
        at org.apache.poi.ss.formula.functions.AbstractNumericTestCase.assertEquals(AbstractNumericTestCase.java:72)
        at org.apache.poi.ss.formula.functions.TestMathX.testRoundDown(TestMathX.java:754)

asfgit pushed a commit that referenced this pull request May 16, 2017

@FishMeat

This comment has been minimized.

Show comment
Hide comment
@FishMeat

FishMeat May 16, 2017

@onealj
Maybe I was wrong. How about

public void testRoundDown() {
    double d = 3987 * 0.2;
    int p = 2;
    assertEquals("round ", 797.4d, MathX.roundDown(d, p));
}

FishMeat commented May 16, 2017

@onealj
Maybe I was wrong. How about

public void testRoundDown() {
    double d = 3987 * 0.2;
    int p = 2;
    assertEquals("round ", 797.4d, MathX.roundDown(d, p));
}
@onealj

This comment has been minimized.

Show comment
Hide comment
@onealj

onealj May 16, 2017

Contributor

I verified that POI currently has the rounding error described above (RoundUp( 3987 * 0.2, 2 ))

I added a disabled unit test https://svn.apache.org/viewvc?view=revision&revision=1795265

Contributor

onealj commented May 16, 2017

I verified that POI currently has the rounding error described above (RoundUp( 3987 * 0.2, 2 ))

I added a disabled unit test https://svn.apache.org/viewvc?view=revision&revision=1795265

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Jul 3, 2017

Contributor

@onealj the change from @FishMeat seems to fix the tests you added - is there a reason not the apply the change?

Contributor

pjfanning commented Jul 3, 2017

@onealj the change from @FishMeat seems to fix the tests you added - is there a reason not the apply the change?

@onealj

This comment has been minimized.

Show comment
Hide comment
@onealj

onealj Jul 3, 2017

Contributor

Nope. I have been travelling. If the patch looks good to you, merge it.

Contributor

onealj commented Jul 3, 2017

Nope. I have been travelling. If the patch looks good to you, merge it.

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Jul 3, 2017

Contributor

@onealj I'll merge now - thanks for verifying

Contributor

pjfanning commented Jul 3, 2017

@onealj I'll merge now - thanks for verifying

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Jul 3, 2017

Contributor

The commented out tests pass with the suggested change but unfortunately, other tests fail.

Contributor

pjfanning commented Jul 3, 2017

The commented out tests pass with the suggested change but unfortunately, other tests fail.

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Jul 12, 2017

Contributor

@onealj This change looks like it should be made. It feels like we need to convert the number away from its double precision value before applying the rounding.
The test in TestMathX that fails uses the value 0.049999999999999975 but if I paste this into Excel it truncates it to 0.0499999999999999. So it appears to me that the test is not accurate.
It seems better that we allow NumberToTextConverter.toText to convert it to 0.05 and then apply the roundDown function.

Contributor

pjfanning commented Jul 12, 2017

@onealj This change looks like it should be made. It feels like we need to convert the number away from its double precision value before applying the rounding.
The test in TestMathX that fails uses the value 0.049999999999999975 but if I paste this into Excel it truncates it to 0.0499999999999999. So it appears to me that the test is not accurate.
It seems better that we allow NumberToTextConverter.toText to convert it to 0.05 and then apply the roundDown function.

@onealj

This comment has been minimized.

Show comment
Hide comment
@onealj

onealj Jul 12, 2017

Contributor

Sure. I'm not sure in what context the 0.4999... unit test was written. I don't know if Excel is consistent across formats (xls/xlsx), versions (2007, 2010, ...) and OS.

Applying the suggested patch and changing the existing 0.49999 test would mean POI behavior is less surprising and closer to Excel than at present.

Contributor

onealj commented Jul 12, 2017

Sure. I'm not sure in what context the 0.4999... unit test was written. I don't know if Excel is consistent across formats (xls/xlsx), versions (2007, 2010, ...) and OS.

Applying the suggested patch and changing the existing 0.49999 test would mean POI behavior is less surprising and closer to Excel than at present.

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Jul 12, 2017

Contributor

What if we make the change and add a system property that allows MathX to run as before?
This allows people to try the change but gives us a fallback if people find the changes cause them trouble.

Contributor

pjfanning commented Jul 12, 2017

What if we make the change and add a system property that allows MathX to run as before?
This allows people to try the change but gives us a fallback if people find the changes cause them trouble.

@onealj

This comment has been minimized.

Show comment
Hide comment
@onealj

onealj Jul 13, 2017

Contributor

@pjfanning probably not necessary. The ROUND() function probably wasn't designed to be used near the system's floating point precision. The way we evaluate formulas using Java doubles instead of Excel numbers will introduce its own discrepancies on the same order of magnitude as system floating point precision. Even if we made ROUND() exactly match Excel, the rest of the formula may not be, and I don't think anyone is asking us to make every NumberEval have the same rounding error properties as Excel (which may have its own quirks, not an arbitrary precision expression).

Contributor

onealj commented Jul 13, 2017

@pjfanning probably not necessary. The ROUND() function probably wasn't designed to be used near the system's floating point precision. The way we evaluate formulas using Java doubles instead of Excel numbers will introduce its own discrepancies on the same order of magnitude as system floating point precision. Even if we made ROUND() exactly match Excel, the rest of the formula may not be, and I don't think anyone is asking us to make every NumberEval have the same rounding error properties as Excel (which may have its own quirks, not an arbitrary precision expression).

@asfgit asfgit closed this in 08a624f Jul 13, 2017

@FishMeat FishMeat deleted the FishMeat:patch-3 branch Jul 13, 2017

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