GNOME Bugzilla – Bug 696575
New Report: Asset Performance
Last modified: 2018-06-29 23:14:29 UTC
Created attachment 239802 [details] [review] Scheme Source File Hi, I have written a report that gives me an idea how good some of my financial products are performing, see the draft version of a usage description below. This ticket asks for checking if this new report could be made available to other users, e.g. by including it as standard report. See below for a description of what the report shall achieve. Currently this report is installed under Reports->Assets&Liabilities. Usage (draft) This Report is intended to measure the performance of asset accounts. The main idea is to visualize the performance of financial programs like life insurances or fonds based saving plans in comparison with simple saving accounts. Nevertheless, this report can be run against any account. The performance is measured as interest rate that should have been applied to an imaginary savings account leading to the same result as shown by the selected real account. There are two measurements included in this report: One on a per year basis, omitting the history of previous years, and the other one including the history from start of the performance measurement time period until the year of evaluation, called accumulated result (accumulated over several years). The per-year-result (yearly result) evaluates each year of the measurement time period in an isolated view. Each year has a balance at the beginnging of the year, a balance at the end of the year, and deposits into the selected accounts throughout the year. The exact time of the deposits will be discarded, instead, it is assumed that the deposits have been done on a monthly basis with the average value (total deposit / 12 ). The result of this measurement is the calculated interest rate of a simple savings account with the same performance. Example: Only one account is selected, the measurement period has been adjusted to span over only one year. The starting balance at the beginning is 100, in April 200 have been dosited, in October 140 have been withdrawn, and one year after the beginning of the measurement period the balance is 110. The algorithm tranlates the deposits and withdrawals into a total deposit of 60 (=200-140) split into 12 monthly deposits of 5. In this example the total of the balance at year start (100) plus the total deposits (60) are greater then the balance at year end (110), meaning that a corresponding saving account has a negative performance with a negative interest rate, i.e. instead of receiving interest fee payments have been made. The accumulated result takes all years before the year of evaluation and the year of evaluation into account. Deposits within of all the years up to the year of evaluation are split into equally large deposits per year. The resulting interest rate applies for all years from the start of the measurement period until the current year of evaluation. Example: Only one account is selected, the measurement period has been adjusted to span over two years. The starting balance at the beginning is 100, in April of the first year 200 have been dosited, in October of the second year 140 have been withdrawn, one year after the beginning of the measurement period (end of first year) the balance is 110, at the end of the second year the balance is 180. For the first year, the algorithm assumes that a deposit of 200 has been done at the beginning of the year, so the interest will be calculated on a starting balance of 300 (=100+200). With a year-end balance of 110 the performance value will be negative, in fact almost the whole deposit of this year has been lost giving a corresding negative result for the first year. For the second year, the algorithm sees a total deposit of 60 (=200-140) and assumes that 30 of those have been available at the start of the first year, and the other 30 at the beginning of the second year. The resulting interest that is valid for both years is based on three parts: Interest paid on the starting balance of 130 (100+30) for the first year, interest paid for the result reached until the beginning of the second year (130 + (interest on 130) ), and finally interest on the second year's deposit of 30. The interest rate is the same for all three parts and the algorithm finds the suitable interest rate that leads to the final balance of 180. NOTE: Deposits must come from a BANK account (meaning, accounts that have been set to type BANK in GnuCash). This is to cover the case that you get e.g. payments from the state into the selected account as for public subsidized programs like old age pension saving plans, and which should be booked in corrsponding income accounts. Only payments from the bank accounts are seen as personal contribution, and payments from outside the bank accounts are treated as additional interest payments.
Comment on attachment 239802 [details] [review] Scheme Source File Thank you for your contribution. I don't manage assets in GnuCash and don't know much about that. So I'm not really well suited to evaluate what your report is, well, reporting. But I do have a few technical points I'd like you to look into first: 1. You provide some usage information, which is very nice. The way you do this though has some issues. a. You define the usage strings early in the report. Some of the paragraphs turn out to be very long lines. Can you please wrap these lines to a more manageable line widths ? Typically lines are wrapped somewhere between 72 and 80 characters. You don't need to do this for the whole code, but at least for the very long definitions. You can wrap a very long string by adding a backslash and a new line like so for example: (define myvar "A line I want\ to wrap") b. You haven't marked these lines as translatable, so the usage will only appear in English. You don't have to worry about the actual translation.Translators will take care of this at some point, though you are of course welcome to contribute translations as well. But you should wrap your definitions in a N_ construct. For example (define myvar (N_ "my definition\ goes here")) You can look at other reports for more examples. 2. I see several more translation related issues: you are often using (_ and ) around guile code. For example (_ (string-append "Performance Report Version " version)) Those are problematic for translation. Only "Performance Report Version" should be marked for translation. Please check your report, it has many of these and similar constructs. Apart from this, it looks very cleanly written !
Created attachment 240113 [details] [review] Scheme Code Version 0.10 - re-formatting text strings to be translatable - adding more comments describing the algorithm
Hi Geert, thanks for the hint, indeed I did not think about the translations at all. Hopefully I covered all places where a change is needed.
Comment on attachment 240113 [details] [review] Scheme Code Version 0.10 Hi Carsten, Sorry for the long delay. This bug managed to escape my attention. Thanks for making the adjustments. It's much more readable now. I have another few comments, in no particular order: - (nitpick category) the file name. Your report is an asset performance report. You have chosen performance-report.scm as name. However, most reports don't use "report" in their name. From their location in the filesystem, it's fairly obvious this file is a report. Adding "asset" in the filename would give it a more precise name. Unless the performance can also be measured from accounts other than assets ? - Perhaps similarly the menu name should be "Asset Performance" instead of "Performance Report" for the same reasons. - I think this report falls in the standard reports category ? Together with the other asset reports ? I had added it in that directory at least, but this failed to load. In that case the module path should also include "standard-reports". - There are a few sections in the code you have borrowed from other scm files (report-utils for example). It would be nice if you can refactor your code to actually use the functions directly from report-utils instead of copying them. If you need variations on existing functions, please check if you can make the base function more generic to work for all situations, otherwise add a variant, but preferably in the original file. For your references to report-utils that file is an obvious candidate. The part(s) you copied from the cash-flow.scm may need to move to a common file as well, if there sufficient overlap. I haven't looked into the detail too much. This is just general advice. I would have included your report as is with the suggestion to work on the above comments in subsequent patches. Unfortunately, the report crashes GnuCash when I try to run it with only one Bank account selected. This will have to get fixed first.
This is the guile trace from the above mentioned crash: In ice-9/boot-9.scm: 157: 19 [catch #t #<catch-closure 10e8a00> ...] In unknown file: ?: 18 [apply-smob/1 #<catch-closure 10e8a00>] In ice-9/boot-9.scm: 157: 17 [catch #t #<catch-closure 2680dc0> #<catch-closure 2680da0> #f] In unknown file: ?: 16 [apply-smob/1 #<catch-closure 2680dc0>] In ice-9/boot-9.scm: 171: 15 [with-throw-handler #t #<catch-closure 2680c60> #<catch-closure 2680c40>] In unknown file: ?: 14 [apply-smob/1 #<catch-closure 2680c60>] ?: 13 [call-with-input-string "(gnc:report-run 1)" ...] In ice-9/boot-9.scm: 2320: 12 [save-module-excursion #<procedure 5389db0 at ice-9/eval-string.scm:65:9 ()>] In ice-9/eval-string.scm: 44: 11 [read-and-eval #<input: string 7e31410> #:lang ...] 37: 10 [lp (gnc:report-run 1)] In report.scm: 754: 9 [gnc:report-run 1] In ice-9/boot-9.scm: 157: 8 [catch ignore #<procedure 5389ba0 at gnucash/main.scm:102:4 ()> ...] In unknown file: ?: 7 [lazy-catch #t #<procedure 5389b40 at gnucash/main.scm:104:18 ()> ...] In ice-9/boot-9.scm: 171: 6 [with-throw-handler #t #<catch-closure 26802c0> #<catch-closure 26802a0>] In unknown file: ?: 5 [apply-smob/1 #<catch-closure 26802c0>] In report.scm: 758: 4 [#<procedure 5389bd0 at report.scm:755:5 ()>] 737: 3 [gnc:report-render-html # #t] In html-document.scm: 196: 2 [gnc:html-document-render # #t] In ice-9/boot-9.scm: 102: 1 [#<procedure 5385800 at ice-9/boot-9.scm:97:6 (thrown-k . args)> vm-error ...] In unknown file: ?: 0 [apply-smob/1 #<catch-closure 26802a0> vm-error ...]
ooh, even a crash ... not good. I was anyway intending to rework on this one, now that I have studied some more of the existing reports. I strongly assume that there is quite a bit of room left for improvement to get it more aligned. That will take some time, but I guess this is not really urgent. I'll be back ...
Created attachment 272149 [details] [review] Patch to be applied on GnuCash 2.6.2 or later Here comes the second try, including all changes for integration. Hopefully no crash anymore. The report is now called Asset Performance. But it still resides under the sub-menu "Assets&Liabilites" as it is meant specifically for asset accounts.
Created attachment 272150 [details] [review] Patch to be applied on gnucash-docs 2.6.2 or later After reading my old documenation and hints for how to apply this report, I realized that it does not make sense trying to explain it without pictures. Here is a patch for the gnucash-docs repository that also includes pictures. Hopefully it includes enough info to enable other users to make use of this report. Note: For this I inserted level 2 headlines, so the reports now can be found in the contents list for chapter 9.
Created attachment 272151 [details] Example file If you want to try the report just to see it is not doing something bad, you can use this example file. If you set the options to - Start on 2000-01-01 - End on 2004-12-31 - and select the account "Insurance1" you should see the picture as shown in the gnucash-docs patch.
Created attachment 272195 [details] overwrite the asset-performance.scm from the patch with this file Hi Geert, anticipating your comments ;-) - I took out the stupid background color - I changed to heading to read "Asset Performance Report" instead of "GnuCash Performance Report" - I made the report footprint optional as just done for the budget barchart report Just apply the patch as uploaded above and copy this file over the file created by the patch.
Created attachment 273579 [details] [review] Patch to be applied on GnuCash 2.6.2 or later Clean up: New patch attachment to include the changes as mentioned in the previous comment. Former attachments 272149 and 272195 become obsolete. That should be the final version of my proposal. ... hopefully ...
Thank you for your improvements. I will certainly look at it in the near future. I don't have time right now. I noticed you marked the bug as "Resolved-Fixed". That should only be done once the code is included in the gnucash repository. Closed reports are completely ignored by developers :) I'll reopen it so your work won't get lost.
oops, good to know :-)
Hi Carsten, I have applied your patch and ran the report. With no accounts selected it works fine (but obviously there is no data to display). Once I select at least one account, the report results in an error. This is printed to the command line: In /home/janssege/Development/Installs/gnucash-f20-master/share/gnucash/guile-modules/gnucash/report/standard-reports/asset-performance.scm: 476: 19 [listOfYears 1.0 31.1 8085] 476: 18 [listOfYears 1.0 31.1 8086] 476: 17 [listOfYears 1.0 31.1 8087] 476: 16 [listOfYears 1.0 31.1 8088] 476: 15 [listOfYears 1.0 31.1 8089] 476: 14 [listOfYears 1.0 31.1 8090] 476: 13 [listOfYears 1.0 31.1 8091] 476: 12 [listOfYears 1.0 31.1 8092] 476: 11 [listOfYears 1.0 31.1 8093] 476: 10 [listOfYears 1.0 31.1 8094] 476: 9 [listOfYears 1.0 31.1 8095] 476: 8 [listOfYears 1.0 31.1 8096] 476: 7 [listOfYears 1.0 31.1 8097] 476: 6 [listOfYears 1.0 31.1 8098] 476: 5 [listOfYears 1.0 31.1 8099] 476: 4 [listOfYears 1.0 31.1 8100] 476: 3 [listOfYears 1.0 31.1 8101] 476: 2 [listOfYears 1.0 31.1 8102] In ice-9/boot-9.scm: 102: 1 [#<procedure 3b97d00 at ice-9/boot-9.scm:97:6 (thrown-k . args)> vm-error ...] In unknown file: ?: 0 [apply-smob/1 #<catch-closure 3b9ae00> vm-error ...] * 17:07:14 WARN <gnc.report.core> Failure running report: Function: vm-run, vm-error In /home/janssege/Development/Installs/gnucash-f20-master/share/gnucash/guile-modules/gnucash/report/standard-reports/asset-performance.scm: 476: 19 [listOfYears 1.0 31.1 8095] 476: 18 [listOfYears 1.0 31.1 8096] 476: 17 [listOfYears 1.0 31.1 8097] 476: 16 [listOfYears 1.0 31.1 8098] 476: 15 [listOfYears 1.0 31.1 8099] 476: 14 [listOfYears 1.0 31.1 8100] 476: 13 [listOfYears 1.0 31.1 8101] 476: 12 [listOfYears 1.0 31.1 8102] In ice-9/boot-9.scm: 102: 11 [#<procedure 3b97d00 at ice-9/boot-9.scm:97:6 (thrown-k . args)> vm-error ...] In unknown file: ?: 10 [apply-smob/1 #<catch-closure 3b9ae00> vm-error ...] In /home/janssege/Development/Installs/gnucash-f20-master/share/gnucash/guile-modules/gnucash/main.scm: 98: 9 [dumper vm-error vm-run "VM: Stack overflow" ()] In unknown file: ?: 8 [display-error #<stack 2bc0930> #<output: file /dev/pts/10> ...] In ice-9/boot-9.scm: 106: 7 [#<procedure 3b97d00 at ice-9/boot-9.scm:97:6 (thrown-k . args)> wrong-number-of-args ...] 102: 6 [#<procedure 3b97d80 at ice-9/boot-9.scm:97:6 (thrown-k . args)> wrong-number-of-args ...] In unknown file: ?: 5 [apply-smob/1 #<catch-closure 3b85200> wrong-number-of-args ...] In ice-9/boot-9.scm: 106: 4 [#<procedure 3b97d00 at ice-9/boot-9.scm:97:6 (thrown-k . args)> wrong-number-of-args ...] 106: 3 [#<procedure 3b97d80 at ice-9/boot-9.scm:97:6 (thrown-k . args)> wrong-number-of-args ...] 65: 2 [abort-to-prompt catch3092 wrong-number-of-args ...] 102: 1 [#<procedure 3b97d80 at ice-9/boot-9.scm:97:6 (thrown-k . args)> vm-error ...] In unknown file: ?: 0 [apply-smob/1 #<catch-closure 3b85200> vm-error ...] I suspect this only occurs in guile 2 and probably because you are recursing over some strings at some point. I had to fix a similar issue right before 2.6.3 was released (see commit a48e656eee070b238cbbc43ae1773f09ccb880fe). I'll also add a conversation I had regarding this issue on the #guile irc channel that may help you find your issue: [Tuesday 25 March 2014] [09:41:03] <gjanssens> Hi, I'm looking at an issue with string-append in guile 2 [Tuesday 25 March 2014] [09:41:32] <gjanssens> I have created a simple code snippet to illustrate my problem: http://pastebin.com/RtyUCm5d [Tuesday 25 March 2014] [09:41:58] <gjanssens> Basically I'm trying to string-append a long list of strings [Tuesday 25 March 2014] [09:42:15] <gjanssens> The test script generates a list of one million strings [Tuesday 25 March 2014] [09:42:33] <gjanssens> And then proposes three ways to concatenate them all together [Tuesday 25 March 2014] [09:43:03] <gjanssens> Using "apply" will result in as vm-run "VM: Stack overflow" error [Tuesday 25 March 2014] [09:43:33] <gjanssens> Using "reduce-right" will as well but with a small delay [Tuesday 25 March 2014] [09:44:21] <gjanssens> "reduce" just seems to run forever, I have aborted the run after 40 minutes. At that point about 560.000 strings were appended together [Tuesday 25 March 2014] [09:44:35] <gjanssens> I don't know if it was stuck there or just slowly proceeding [Tuesday 25 March 2014] [09:45:00] <gjanssens> So I wonder how I can get this working ? [Tuesday 25 March 2014] [09:45:29] <gjanssens> Some background: I discovered this issue in GnuCash [Tuesday 25 March 2014] [09:46:07] <gjanssens> At some point that program has built a tree of strings representing a large html file [Tuesday 25 March 2014] [09:46:27] <gjanssens> To actually generate the html, all these strings are appended together [Tuesday 25 March 2014] [09:46:44] <gjanssens> This has always worked fine with guile 1.8 using the apply method [Tuesday 25 March 2014] [09:47:13] <gjanssens> But with the switch to guile 2 we are getting user reports of the above vm-run error [Tuesday 25 March 2014] [09:47:23] <gjanssens> I'm looking for a way to avoid this. [Tuesday 25 March 2014] [09:51:59] Quit vicenteH (~user@143.79.21.95.dynamic.jazztel.es) has left this server (Ping timeout: 246 seconds). [Tuesday 25 March 2014] [09:53:51] <wingo> hi [Tuesday 25 March 2014] [09:54:07] <wingo> gjanssens: there are a few things there [Tuesday 25 March 2014] [09:54:41] <wingo> the first one is to use string-concatenate to append a list of strings. [Tuesday 25 March 2014] [09:54:52] <wingo> i guess that's the short answer [Tuesday 25 March 2014] [09:55:14] <wingo> the problem with appending them pairwise is that you generate O(n^2) garbage [Tuesday 25 March 2014] [09:55:32] <wingo> string-concatenate precomputes the buffer size needed, and so it's O(n) [Tuesday 25 March 2014] [09:56:25] <wingo> using string-append would be fine but as you see it runs out of stack, and (apply string-append list) uselessly unrolls the list onto the stack for the call, only to cons it up again inside string-append -- that's why string-concatenate is better, as it takes a list directly [Tuesday 25 March 2014] [09:57:19] <wingo> you might be interested to know that we have removed the stack limit in master, but that's not really relevant to your needs, and anyway the answer is still the same -- string-concatenate [Tuesday 25 March 2014] [09:58:20] <wingo> w.r.t. guile 1.8 versus 2.0 -- in guile 1.8 all function calls would cons their arguments into a list; that was inefficient, and so in guile 2.0 argument passing happens on a stack [Tuesday 25 March 2014] [09:59:13] <wingo> but guile 1.8 did have the advantage that because it used lists, argument lengths were effectively unlimited, whereas they were limited in 2.0 by the stack size -- but that limit as I said is going away in 2.2 so we come full circle :) [Tuesday 25 March 2014] [09:59:22] <wingo> gjanssens: let me know if you need any more details You may want to ask about this on the above mentioned list. The people there are usually very willing to help (with wingo leading the pack).
Comment on attachment 273579 [details] [review] Patch to be applied on GnuCash 2.6.2 or later I revisited your patch with the intention to fix the guile 2 issue. Only it turns out there is no guile 2 issue. The problem is in the implementation of your getListOfYears function. In line 483 and 484 you are extracting the year of your start and end date in a way that is not localized. Your code assumes the date will start with the year. However the date format in my locale is dd.mm.yyyy. So your code effectively extracts dd.m in my locale. This explains the long lists of 476: 19 [listOfYears 1.0 31.1 8085] in my backtrace. "1.0" is an improperly extracted year from date value "01.01.2004" and likewise "31.1" are the first 4 characters from "31.12.2004". You will need to find a reliable method of extracting the year from the dates that works for all supported locales. Perhaps the gnucash code has some function to convert the date into iso format (yyyy-mm-dd) ? Just an idea, I haven't investigated further...
Revisiting this - there's a procedure called gnc:date-get-year defined in date-utilities.scm - it takes a datevec and outputs a number. Would this help?
Reassign version to 2.4.x so that individual 2.4 versions can be retired.
This report can be fixed. All it needs is modification of getListOfYears to: (define getListOfYears (lambda (start-date end-date) (define listOfYears (lambda (fy ly ny) (if (eq? (- ly fy) ny) (list (+ fy ny)) (cons (+ fy ny) (listOfYears fy ly (+ ny 1)))))) (let ((first-year (gnc:timepair-get-year start-date)) (last-year (gnc:timepair-get-year end-date)) (number-of-years 0)) (listOfYears first-year last-year number-of-years)))) Perhaps OP could amend?
Thanks for the hint (and solution). After some more years of experience with Guile I feel the whole report might require some rework - and shift it to the future as other topics became more urgent. I will return to this, but probably not before end of this year Thanks again for having an eye on this.
Created attachment 366070 [details] [review] patch to be applied on GnuCash 2.7.2 or later finally a made it: here a re-worked version to be used with 2.7.2 or later I also obsolete the gnucash-docs patch - will make a new one later. For now I have extended the report description in the source file.
We're about to merge a large change that does away with gnc:timepair, replacing it with time64. This change won't merge with that. See https://github.com/christopherlam/gnucash/commit/9acb3bb6ce284a93731f0814d0740763806c7c3d for the replacement functions. The other is a spelling error: "singel" should be "single".
Created attachment 372487 [details] [review] patch to be applied on Gnucash 3.0 or later next try this version of the patch also includes tests for this report - duration on my end ~9s
Created attachment 372488 [details] Simple Example Data File I also add a new example file which is more simple than the previous one and which matches with the examples given in the report description (at the beginning of the asset-performance.scm file)
Hi Carsten- Do you have a github login? It's easier to offer opinionated reviews! 1. It's nice to see srfi64 being used. I've designed gnc:test-runner to count number of passes and failures, and for test-runner-on-final! function to return #f if number-of-failures > 0. So there should be little need to use (test-passed?), as long as the last statement in (run-test) is (test-end "Testing/Temporary/test-asset-performance") 2. There is an effort to eliminate gnc-numeric in scheme. xaccSplitGetAmount xaccSplitGetValue will return scheme exact numbers directly (e.g. 105/2 to signify $52.50) and can be added/multiplied/divided; there's no need to convert to doubles nor handle numerators/denominators anymore. So, for example, I believe (gnc:account-get-balance-at-date) should return a scheme exact number. 3. typo in test - should be "make-yearly-transactions" 4. instead of (list->vector (list ...)) better do (vector ...) directly? 5. I'd convert (+ ... 1) to (1+ ...), (eq? lst '()) to (null? lst), etc because I'm picky 6. sum-of-long-years-before seems to handle leapyear dates before & after 1970 differently? I'd think there must be a bug there. Also (+ 0 ...) ??? 7. I think the (_ ...) specifier is encompassing the whole complex string and should be applied to the smaller string "Result Corridor: " or "Fiscal Period: " only 8. I think seasoned schemers would avoid set! calls as far as possible. Overall the code looks very clean - as a result I've been able to understand enough to offer a quick review! I don't understand the calculations so I'll trust they are accurate.
P.S. would you mind if I were to amend the report to use more API and SRFI constructs, and create a PR? My code style will be much more concise than yours, however, I am confident I can fix it up and submit a PR for inclusion. I envisage only the numeric calculations will survive unscathed from my fixes...
Hi Chris, that is a great offer. Up to now I learn from the other reports that I come across. Once I found a working method I tend to stick to it instead of validating if it is the best (or proper) one. Feel free to go ahead as you wish, meanwhile I will read and learn about github.
Hi Carsten I've been trying to make it work; unfortunately it keeps crashing. Perhaps I'll try analysing from your numeric calculations and may need to recreate from scratch. It seems a nice addition to the library. Perhaps you could provide a bigger example datafile, and the full report generated. It can also help to provide an Excel/Libreoffice spreadsheet to ensure I understand the numerics. I'd hope that the calculations will be correct with stock accounts too. I also notice the following lines - are you exporting the results to another report? (export get-account-result-list) (export getAccountByFullName) (export get-performance-list)
hm, crashing, not good - I guess (hope) the crashing is connected to some changes you have applied ... Not sure if a bigger datafile will help, in fact, I would think a smaller data file is more useful for trouble shooting. Not? The export of those procedures is needed to enable testing them "from outside". The test files coming along with the patch are accessing these procedures.
GnuCash bug tracking has moved to a new Bugzilla host. The new URL for this bug is https://bugs.gnucash.org/show_bug.cgi?id=696575. Please continue processing the bug there and please update any external references or bookmarks.