Category charts upgrade to chart allocation ratios instead of absolute values. #1272

exxus · 2022-02-09T11:40:30Z

I created a patch to show ratios in the barchart.
This is useful if you want to maintain a certain ratio between different asset classes, e.g. 60:40.
I contribute this, since I think it can help other users as well.
Before this can be merged, maybe some documentation needs to be added.
Thank you all for this really helpful piece of software.

Best regards
exxus

christopherlam

Some comments. This is an interesting chart modification, and needs further testing. I've also seen div/0 errors but cannot debug this at this time.

gnucash/report/reports/standard/category-barchart.scm

christopherlam · 2022-02-09T14:05:48Z

gnucash/report/reports/standard/category-barchart.scm

@@ -34,6 +34,13 @@
 (use-modules (gnucash app-utils))
 (use-modules (gnucash report))

+;; useful functions
+(define divide-number (lambda (x y) (gnc-numeric-div  x  y GNC-DENOM-AUTO (logior (GNC-DENOM-SIGFIGS 6) GNC-RND-ROUND) )))


Don't need this, use / directly

If I use / directly, it will lead to overflow errors due to division by 0.
I have some time spans where all assets are 0. In this case, ratios make no sense.
But using the gnc-numeric-divide function, the report does not crash but shows 0% for the ratio of all assets.

Ok. I'm not sure of all the edge cases, but I'd define a more descriptive function to explicitly demonstrate that we're handling div/0 gracefully. IIUC gnc_numeric_div div/0 will log an error.

(define (safe-/ x y) (if (zero? y) 0 (/ x y)))

gnucash/report/reports/standard/category-barchart.scm

christopherlam · 2022-02-10T14:38:50Z

gnucash/report/reports/standard/category-barchart.scm

@@ -552,6 +570,7 @@ developing over time"))
                              (show-fullname? (gnc-account-get-full-name acct))
                              (else (xaccAccountGetName acct))))
                      (amounts (map gnc:gnc-monetary-amount (cadr series)))
+                      (percentage (map (lambda (x grandt)  (divide-number x grandt ) )  amounts grandts)) 


Suggested change

(percentage (map (lambda (x grandt) (divide-number x grandt ) ) amounts grandts))

(percentage (map divide-number amounts grandts))

christopherlam · 2022-02-10T14:43:07Z

Finally this modification would benefit from tests in test-standard-category-report.scm. Would you be able to watch the userlist and buzgilla for bug reports and respond? Alternatively a description for the release notes, which include hints how to handle edge cases will be needed. Merging this change will undoubtly generate queries and the core developers do not have all answers.

gnucash/report/reports/standard/category-barchart.scm

exxus · 2022-02-10T20:53:30Z

Thank you for all the comments and suggestions. I updated the branch accordingly.

Finally this modification would benefit from tests in test-standard-category-report.scm. Would you be able to watch the userlist and buzgilla for bug reports and respond? Alternatively a description for the release notes, which include hints how to handle edge cases will be needed. Merging this change will undoubtly generate queries and the core developers do not have all answers.

I am not able to read all the mailing list, but I will try to reply and maintain the option when I get notifactions e.g. on github.
I agree that is testcase is useful, I looked at the mentioned file, but couldn't figure out how to add a test.
Is there testing database with which I can test edge cases?

I guess the most significant edge cases are negative values, because there is no clear understanding to me how to divide e.g. a cake with negative pieces. I can write an explanation with the intended use case and what happens in case of negative values.

I mainly use it to maintain a certain ratio between some of my asset classes (e.g. investment funds and fixed-interest securities). In the hope other will benefit from it as well, I wanted to share it.
But if it is too complicated to maintain, understand or support or if it is not in the scope of the reports provided with gnucash, don't hesitate to close the pull request.

Thanks again for all the helpful comments.

christopherlam · 2022-02-11T02:39:45Z

Finally this modification would benefit from tests in test-standard-category-report.scm. Would you be able to watch the userlist and buzgilla for bug reports and respond? Alternatively a description for the release notes, which include hints how to handle edge cases will be needed. Merging this change will undoubtly generate queries and the core developers do not have all answers.

For copyright and attribution we may wish to add your github handle onto the report source code.

I am not able to read all the mailing list, but I will try to reply and maintain the option when I get notifactions e.g. on github. I agree that is testcase is useful, I looked at the mentioned file, but couldn't figure out how to add a test. Is there testing database with which I can test edge cases?

test-standard-category-report.scm calls env-create-daily-transactions in test-engine-extras to create the book, dumps the html somewhere in /tmp/ and tests the resulting table contents using sxml. You may use the same test data, or (better) create custom ones.

I guess the most significant edge cases are negative values, because there is no clear understanding to me how to divide e.g. a cake with negative pieces. I can write an explanation with the intended use case and what happens in case of negative values.

Yes it would be useful to write an explanation for the release notes. I think the most severe edge case will be how to handle mixed-account-type hierarchies such as Income[INCOME]:Salary[INCOME]:PAYETaxes[EXPENSE] for example, whereby both Salary and PAYETaxes are populated with splits (and the account balances may become negative!). But the existing reports are likely to fail too.

I mainly use it to maintain a certain ratio between some of my asset classes (e.g. investment funds and fixed-interest securities). In the hope other will benefit from it as well, I wanted to share it. But if it is too complicated to maintain, understand or support or if it is not in the scope of the reports provided with gnucash, don't hesitate to close the pull request.

I think it is an interesting chart, and as bonus it is a single-option upgrade to an existing report.

christopherlam · 2022-02-11T02:40:34Z

gnucash/report/reports/standard/category-barchart.scm

+(define (safe-/ x y)
+  (if (and (zero? x) (zero? y)) 0 (/ x y)))


Are you sure both x and y being 0 is the only div/0? What if the assets have only two accounts: cash $400 and loan -$400?

This is the part where the abs value will make y 800. In this case it makes no difference. Since if x=0, y will also always be 0. We can also stick with (if (zero? y) 0 (/ x y))). I just added the check for x, because for x not 0, a divide by 0 error in this case would show that something is wrong.

Since if x=0, y will also always be 0.

There is nothing in this function definition that guarantees this so your statement must be based on how this function is used. That's slightly risky because that opens doors to mistakes in future changes of the code.

We can also stick with (if (zero? y) 0 (/ x y))). I just added the check for x, because for x not 0, a divide by 0 error in this case would show that something is wrong.

The added check adds nothing. The function will return 0 in both implementations so there's no way your calling code can tell the difference.

There is nothing in this function definition that guarantees this so your statement must be based on how this function is used. That's slightly risky because that opens doors to mistakes in future changes of the code.

That is true, only in my use case if y=0, also x will be 0.

Since if x=0, y will also always be 0.

Sorry, that was is mistake. In my usecase: y=abs(x_1)+...+abs(x_N). So if y=0, all x must have been 0.

The added check adds nothing. The function will return 0 in both implementations so there's no way your calling code can tell the difference.

Consider the case y=0, x=1.
(if (zero? y) 0 (/ x y))) will return 0
(if (and (zero? x) (zero? y)) 0 (/ x y))) will fail due to division by 0: (/ 1 0).
In my implementation the case y=0, x=1 can't happen. So if it still occurs, something is wrong and the report will fail.

christopherlam · 2022-02-11T04:33:51Z

While reviewing this PR I think the collector->monetary part to create a gnc-monetary and should be changed to collector->amount, an amount of the known report currency. This will simplify this report a lot. I think this change can be done after merging this PR.

Instead of collector->monetary which takes a collector (bag of monetaries) and returns a monetary {report-currency, amount}, return amount. Simplifies code.

christopherlam

Final comments: I agree it is clever to show negative assets as a negative bar, it may not be strictly legitimate according to formal reporting. Many users are extremely sharp as far as accounting standards are concerned, and will ask questions. So, some documentation, including caveats on edge cases, and description how it handles negative amounts, will be crucial for users to understand its behaviour.

christopherlam · 2022-02-11T14:16:51Z

gnucash/report/reports/standard/category-barchart.scm

                (for-each
                 (lambda (date row)
                   (gnc:html-table-append-row!
                    table
-                    (append (list (make-cell date))
-                            (map make-cell row)
-                            (if cols>1?
-                                (list
-                                 (make-cell (apply gnc:monetary+ row)))
-                                '()))))
+                    (let ((grandt (apply l1norm (map gnc:gnc-monetary-amount row))))
+                     (append (list (make-cell date))
+                             (if ratio-chart? 
+                                 (map (lambda (x) (make-cell-percent (* (safe-/ (gnc:gnc-monetary-amount x) grandt) 100))) row)
+                                 (map make-cell row))
+                             (if cols>1?
+                                 (list
+                                  (make-cell (apply gnc:monetary+ row)))
+                                 '())))))
                 date-string-list
                 (apply zip (map cadr all-data)))


Instead of recreating grandt for each row, it'll be much more clever to reuse the grandts already generated. For that matter, you'll want to modify the for-each as follows:

(for-each (lambda (date row grandt) (gnc:html-table-append-row! ...etc...and use grandt now available for use)) date-string-list (apply zip (map cadr all-data)) grandts)

christopherlam · 2022-02-11T14:21:16Z

gnucash/report/reports/standard/category-barchart.scm

@@ -552,6 +569,7 @@ developing over time"))
                              (show-fullname? (gnc-account-get-full-name acct))
                              (else (xaccAccountGetName acct))))
                      (amounts (map gnc:gnc-monetary-amount (cadr series)))
+                      (percentage (map (lambda (x grandt)  (safe-/ x grandt)) amounts grandts))


This line is ok for readability, but because percentage is only used iff ratios-chart? is #t, it would be marginally more efficient to generate the calls to map and safe-/ only when required; thus:

(gnc:html-chart-add-data-series! chart label (if ratio-chart? (map safe-/ amounts grandts) amounts) color 'stack stack 'fill fill 'urls urls)

…hub.com/exxus/gnucash into exxus-category-barchart-patch

christopherlam · 2022-02-22T13:10:41Z

With 5f06fc9 in maint, this branch is no longer current. Please direct all future discussions in #1274.

exxus added 2 commits February 9, 2022 12:12

Add format style option to html-chart

db3bd37

Added option to show ratios in category-barchart

e82ecab

fellen requested a review from christopherlam February 9, 2022 13:35

christopherlam requested changes Feb 10, 2022

View reviewed changes

exxus force-pushed the category-barchart-patch branch 2 times, most recently from 9d31f77 to 3dc112c Compare February 10, 2022 13:06

christopherlam reviewed Feb 10, 2022

View reviewed changes

gnucash/report/reports/standard/category-barchart.scm Outdated Show resolved Hide resolved

update with some of the requests solved

0074307

exxus force-pushed the category-barchart-patch branch from 3dc112c to 0074307 Compare February 10, 2022 19:57

christopherlam reviewed Feb 11, 2022

View reviewed changes

christopherlam changed the title ~~Category barchart patch~~ Category charts upgrade to chart allocation ratios instead of absolute values. Feb 11, 2022

[net-charts] simplify code

4e950e6

Instead of collector->monetary which takes a collector (bag of monetaries) and returns a monetary {report-currency, amount}, return amount. Simplifies code.

christopherlam approved these changes Feb 11, 2022

View reviewed changes

christopherlam added 3 commits February 12, 2022 13:26

[category-barchart] simplify code

9df126b

Merge branch 'category-barchart-patch' of Gnucash#1272 from git://git…

b93d2b2

…hub.com/exxus/gnucash into exxus-category-barchart-patch

Add test for allocation ratios

c05a26e

exxus mentioned this pull request Feb 15, 2022

Category charts upgrade to chart allocation ratios instead of absolute values. #1274

Merged

christopherlam closed this Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Category charts upgrade to chart allocation ratios instead of absolute values. #1272

Category charts upgrade to chart allocation ratios instead of absolute values. #1272

exxus commented Feb 9, 2022

christopherlam left a comment

christopherlam Feb 9, 2022

exxus Feb 10, 2022

christopherlam Feb 10, 2022

christopherlam Feb 10, 2022

christopherlam commented Feb 10, 2022

exxus commented Feb 10, 2022

christopherlam commented Feb 11, 2022 •

edited

christopherlam Feb 11, 2022

exxus Feb 11, 2022

gjanssens Feb 11, 2022

exxus Feb 15, 2022

christopherlam commented Feb 11, 2022

christopherlam left a comment

christopherlam Feb 11, 2022

christopherlam Feb 11, 2022

christopherlam commented Feb 22, 2022

	(percentage (map (lambda (x grandt) (divide-number x grandt ) ) amounts grandts))
	(percentage (map divide-number amounts grandts))

		(define (safe-/ x y)
		(if (and (zero? x) (zero? y)) 0 (/ x y)))

Category charts upgrade to chart allocation ratios instead of absolute values. #1272

Category charts upgrade to chart allocation ratios instead of absolute values. #1272

Conversation

exxus commented Feb 9, 2022

christopherlam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherlam commented Feb 10, 2022

exxus commented Feb 10, 2022

christopherlam commented Feb 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherlam commented Feb 11, 2022

christopherlam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherlam commented Feb 22, 2022

christopherlam commented Feb 11, 2022 •

edited