Bug 54469 - [PATCH] IPMT, PPMT financial functions implemented and integrated in latest POI
Summary: [PATCH] IPMT, PPMT financial functions implemented and integrated in latest POI
Status: NEEDINFO
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: 3.10-dev
Hardware: PC Linux
: P2 enhancement with 1 vote (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2013-01-22 18:50 UTC by Mike Argyriou
Modified: 2015-08-10 09:17 UTC (History)
0 users



Attachments
diff patch file created from "ant -f patch.xml" on the latest svn POI repository (1.95 KB, application/x-gzip)
2013-01-23 20:35 UTC, Mike Argyriou
Details
Updated FormulaEvalTestData (164.00 KB, application/vnd.ms-excel)
2013-02-13 16:30 UTC, Nick Burch
Details
updated patch (3.25 KB, application/x-gzip)
2013-02-23 23:16 UTC, Mike Argyriou
Details
Main classes for testing ipmt, pmt, etc (678 bytes, application/x-gzip)
2013-02-23 23:18 UTC, Mike Argyriou
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Argyriou 2013-01-22 18:50:02 UTC
I have extended POI to contain the EXCEL fincancial functions
IPMT, PPMT. Just diff and integrate the differences... is this the right place for posting the patched POI?
Comment 1 Mike Argyriou 2013-01-22 19:51:09 UTC
Because the patched POI is too large to attach it here (about 100MB) you can download it from http://uploaded.net/file/ulq8cj21
Comment 2 Nick Burch 2013-01-22 20:50:49 UTC
The patch should normally be just a few kb in size, maybe a few hundred kb with test files. Are you able to check that your patch only has these, and not something else by mistake? (100mb is a pretty huge patch!)
Comment 3 Mike Argyriou 2013-01-22 20:55:11 UTC
Actually I have uploaded all the POI project patched with the new code. Should I upload only the diffs?
Comment 4 Yegor Kozlov 2013-01-23 17:50:53 UTC
yes, you should upload only the diffs. See http://poi.apache.org/guidelines.html#SubmittingPatches

(In reply to comment #3)
> Actually I have uploaded all the POI project patched with the new code.
> Should I upload only the diffs?
Comment 5 Mike Argyriou 2013-01-23 20:01:49 UTC
I have downloaded the latest POI from git, integrated/created the IPMT,PPMT functions and created the patched using "ant -f patch.xml". I have attached the path created by Ant.
Comment 6 Mike Argyriou 2013-01-23 20:35:58 UTC
Created attachment 29887 [details]
diff patch file created from "ant -f patch.xml" on the latest svn POI repository
Comment 7 Mike Argyriou 2013-02-12 18:09:24 UTC
Is there any feedback for this patch?
Comment 8 Nick Burch 2013-02-13 16:30:00 UTC
Created attachment 29949 [details]
Updated FormulaEvalTestData
Comment 9 Nick Burch 2013-02-13 16:31:33 UTC
The patch doesn't include any unit tests, so I tried adding one myself by putting in a few sample formulas using the functions in FormulaEvalTestData.xls

However, when I do that, the unit test fails as POI is unable to correctly evaluate the PPMT function to give the same result as Excel

Could you please look into this, and produce either an updated patch that fixes it, or suggest an alternate test for it?
Comment 10 Mike Argyriou 2013-02-13 16:35:49 UTC
Can you attach the unit test?
Comment 11 Mike Argyriou 2013-02-22 17:30:16 UTC
Nick can you attach the unit tests please so I can check why they fail in order the patch to be integrated to POI? Thank u!
Comment 12 Nick Burch 2013-02-22 17:32:18 UTC
If you grab the POI sourcecode, you'll find the unit test in there already. If you replace FormulaEvalTestData in test-documents with the one attached, and re-run the test with your patch applied you'll see the failure
Comment 13 Mike Argyriou 2013-02-22 17:33:47 UTC
Thank you for your immediate answer! I will take care of it asap...
Comment 14 Mike Argyriou 2013-02-23 23:16:44 UTC
Created attachment 29986 [details]
updated patch

Updated patch....some minor fixes and some typos...

The tests should pass if you set the formatting of the cells in FormulaEvalTestData.xls to more decimal digits... the IPMT test fails because the cell D764 with value 0.01 has two decimals digits...the test returns the value ~0.005 and it fails..increase the cell's number of decimal digits to (e.g.) 4...
Comment 15 Mike Argyriou 2013-02-23 23:18:19 UTC
Created attachment 29987 [details]
Main classes for testing ipmt, pmt, etc

The attachnent contains Main-classes which test the financial functions...check the code please...

place all the *.java in trunk/src/java
Comment 16 Mike Argyriou 2013-02-23 23:19:33 UTC
I have uploaded an updated patch and new convinient manual tests (should not be included in the POI trunk repository)..

Just read the attachments comments..in the nutshell please increase the decimal digitis on the test-xls.

Thanks you
Mike
Comment 17 Yegor Kozlov 2013-03-02 13:22:59 UTC
Partially applied in r1451886 with some tweaks.

Your implementation of PPMT is buggy. If I run PPMT_test I'm getting this:

ppmt=-121.95920277082378 [should be equal to -350.99]

The value -350.99 is correct, I confirmed it in Excel. 

Interestingly that two test cases from http://office.microsoft.com/en-001/excel-help/ppmt-function-HP010342774.aspx pass. 

Please fix PPMT , I guess it a math error and should be easy to detect and fix.

Regards,
Yegor
Comment 18 Dominik Stadler 2013-08-06 05:59:34 UTC
needs info based on previous comment