Skip to content

Commit

Permalink
Fix RATE, PRICE, XIRR, and XNPV Functions (#1456)
Browse files Browse the repository at this point in the history
There were about 20 skipped tests for RATE and PRICE marked
"This test should be fixed". This change does that by fixing
the code for those functions, validating the existing tests,
and adding new ones. XIRR and XNPV are also substantially changed.
As part of this change, the following functions also have minor changes:

  - isValidFrequency
  - COUPDAYBS
  - COUPNUM (additional tests)
  - DB
  - DDB

PhpUnit reports 100% coverage for all the changed functions.

Since I was dealing with skipped tests, I also fixed
tests/PhpSpreadsheetTests/Writer/Xlsx/LocaleFloatsTest,
which was being skipped in Windows. I also delete the temporary
file which it creates.
There is now only one remaining test which is skipped -
ODS Reader is not complete enough to run some tests against it.
Unfortunately, that test is too complicated for me to deal with now.

In researching this change, I found several places in the code where special code was added for Gnumeric claiming:

   - Gnumeric does not handle free-format string dates
   - Gnumeric adds extra options, not available in Excel,
     for the frequency parameter for functions such as YIELD
   - Gnumeric rounds the results for DB and DDB to 2 decimal places

None of these claims is true, at least not on a recent version
of Gnumeric, and the code which supports these differences is removed.
There did not appear to be any tests targeted for
these supposed properties of Gnumeric.

The PRICE function needed relatively minor changes - mostly
additional tests for invalid input. The main problem with the PRICE
tests is that Excel appears to have a bug. The algorithm is published:
https://support.office.com/en-us/article/price-function-3ea9deac-8dfa-436f-a7c8-17ea02c21b0a
The results that Excel returns for basis codes 2 and 3 appear to be
incorrect in many cases. I have segregated these tests into a
new test PRICE3. The results of these tests agree with the published
algorithm, and to the results for LibreOffice and Gnumeric.
The results returned by Excel do not agree with them.
The tests which remain in the test PRICE all use basis codes other
than 2 or 3, and all agree with Excel, LibreOffice, and Gnumeric.

For the RATE function, there appears to be a problem with how the
secant method was implemented. I studied the implementation of RATE
in Python numpy, and adapted its implementation of secant method.
The results now agree with numpy, and, more important, with Excel.

XIRR, which calls XNPV, permits its dates to be earlier than the
start date, whereas XNPV does not. I dealt with this by renaming
the existing XNPV function to xnpvOrdered, adding a parameter to
indicate whether start date has to be earliest. XNPV calls the new
function with that parameter set to TRUE, and XIRR calls it with
the parameter set to FALSE. Some additional error checking was
added to xnpvOrdered, and also to XIRR. XIRR tests benefited
from increasing the value of FINANCIAL_MAX_ITERATIONS.

Finally, since this change is very test-related:
samples/Basic/13_CalculationCyclicFormulae
PhpUnit started reporting an error like "too much regression".
The test deals with an infinite cyclic formula, and allowed
the calculation engine to run for 100 cycles. The actual number of cycles
seems irrelevant for the purpose of this test. I changed it to 15,
and PhpUnit no longer complains.
  • Loading branch information
oleibman authored May 17, 2020
1 parent 8eaceb0 commit 9ae521c
Show file tree
Hide file tree
Showing 10 changed files with 718 additions and 197 deletions.
6 changes: 1 addition & 5 deletions src/PhpSpreadsheet/Calculation/DateTime.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,17 +59,13 @@ private static function dateDiff360($startDay, $startMonth, $startYear, $endDay,
/**
* getDateValue.
*
* @param string $dateValue
* @param mixed $dateValue
*
* @return mixed Excel date/time serial value, or string if error
*/
public static function getDateValue($dateValue)
{
if (!is_numeric($dateValue)) {
if ((is_string($dateValue)) &&
(Functions::getCompatibilityMode() == Functions::COMPATIBILITY_GNUMERIC)) {
return Functions::VALUE();
}
if ((is_object($dateValue)) && ($dateValue instanceof \DateTimeInterface)) {
$dateValue = Date::PHPToExcel($dateValue);
} else {
Expand Down
360 changes: 211 additions & 149 deletions src/PhpSpreadsheet/Calculation/Financial.php

Large diffs are not rendered by default.

60 changes: 54 additions & 6 deletions tests/PhpSpreadsheetTests/Calculation/FinancialTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -436,17 +436,34 @@ public function providerNPV()
*/
public function testPRICE($expectedResult, ...$args)
{
$this->markTestIncomplete('TODO: This test should be fixed');

$result = Financial::PRICE(...$args);
self::assertEqualsWithDelta($expectedResult, $result, 1E-8);
self::assertEqualsWithDelta($expectedResult, $result, 1E-7);
}

public function providerPRICE()
{
return require 'tests/data/Calculation/Financial/PRICE.php';
}

/**
* @dataProvider providerPRICE3
*
* @param mixed $expectedResult
*/
public function testPRICE3($expectedResult, ...$args)
{
// These results (PRICE function with basis codes 2 and 3)
// agree with published algorithm, LibreOffice, and Gnumeric.
// They do not agree with Excel.
$result = Financial::PRICE(...$args);
self::assertEqualsWithDelta($expectedResult, $result, 1E-7);
}

public function providerPRICE3()
{
return require 'data/Calculation/Financial/PRICE3.php';
}

/**
* @dataProvider providerPRICEDISC
*
Expand Down Expand Up @@ -486,8 +503,6 @@ public function providerPV()
*/
public function testRATE($expectedResult, ...$args)
{
$this->markTestIncomplete('TODO: This test should be fixed');

$result = Financial::RATE(...$args);
self::assertEqualsWithDelta($expectedResult, $result, 1E-8);
}
Expand All @@ -506,14 +521,47 @@ public function providerRATE()
public function testXIRR($expectedResult, $message, ...$args)
{
$result = Financial::XIRR(...$args);
self::assertEqualsWithDelta($expectedResult, $result, Financial::FINANCIAL_PRECISION, $message);
if (is_numeric($result) && is_numeric($expectedResult)) {
if ($expectedResult != 0) {
$frac = $result / $expectedResult;
if ($frac > 0.999999 && $frac < 1.000001) {
$result = $expectedResult;
}
}
}
self::assertEquals($expectedResult, $result, $message);
}

public function providerXIRR()
{
return require 'tests/data/Calculation/Financial/XIRR.php';
}

/**
* @dataProvider providerXNPV
*
* @param mixed $expectedResult
* @param mixed $message
*/
public function testXNPV($expectedResult, $message, ...$args)
{
$result = Financial::XNPV(...$args);
if (is_numeric($result) && is_numeric($expectedResult)) {
if ($expectedResult != 0) {
$frac = $result / $expectedResult;
if ($frac > 0.999999 && $frac < 1.000001) {
$result = $expectedResult;
}
}
}
self::assertEquals($expectedResult, $result, $message);
}

public function providerXNPV()
{
return require 'data/Calculation/Financial/XNPV.php';
}

/**
* @dataProvider providerPDURATION
*
Expand Down
3 changes: 2 additions & 1 deletion tests/PhpSpreadsheetTests/Writer/Xlsx/LocaleFloatsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ protected function setUp(): void
{
$this->currentLocale = setlocale(LC_ALL, '0');

if (!setlocale(LC_ALL, 'fr_FR.UTF-8')) {
if (!setlocale(LC_ALL, 'fr_FR.UTF-8', 'fra_fra')) {
$this->localeAdjusted = false;

return;
Expand Down Expand Up @@ -45,6 +45,7 @@ public function testLocaleFloatsCorrectlyConvertedByWriter()

$reader = new \PhpOffice\PhpSpreadsheet\Reader\Xlsx();
$spreadsheet = $reader->load($filename);
unlink($filename);

$result = $spreadsheet->getActiveSheet()->getCell('A1')->getValue();

Expand Down
35 changes: 35 additions & 0 deletions tests/data/Calculation/Financial/COUPNUM.php
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,39 @@
4,
0,
],
[
16,
'1-Apr-2012',
'31-Mar-2020',
2,
0,
],
[
16,
'1-Apr-2012',
'31-Mar-2020',
2,
1,
],
[
16,
'1-Apr-2012',
'31-Mar-2020',
2,
2,
],
[
16,
'1-Apr-2012',
'31-Mar-2020',
2,
3,
],
[
16,
'1-Apr-2012',
'31-Mar-2020',
2,
4,
],
];
Loading

0 comments on commit 9ae521c

Please sign in to comment.