Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
db3bd37
Add format style option to html-chart
exxus Feb 9, 2022
e82ecab
Added option to show ratios in category-barchart
exxus Feb 9, 2022
0074307
update with some of the requests solved
Feb 10, 2022
4e950e6
[net-charts] simplify code
christopherlam Feb 11, 2022
9df126b
[category-barchart] simplify code
christopherlam Feb 12, 2022
b93d2b2
Merge branch 'category-barchart-patch' of #1272 from git://github.com…
christopherlam Feb 12, 2022
c05a26e
Add test for allocation ratios
christopherlam Feb 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 10 additions & 3 deletions gnucash/report/html-chart.scm
View file
Open in desktop
Expand Up @@ -51,6 +51,8 @@
(export gnc:html-chart-set-currency-iso!)
(export gnc:html-chart-currency-symbol)
(export gnc:html-chart-set-currency-symbol!)
(export gnc:html-chart-format-style)
(export gnc:html-chart-set-format-style!)
(export gnc:html-chart-render)
(export gnc:html-chart-set-custom-x-axis-ticks?!)
(export gnc:html-chart-set-title!)
Expand Down Expand Up @@ -144,13 +146,14 @@

(define-record-type <html-chart>
(make-html-chart width height chart-options currency-iso
currency-symbol custom-x-axis-ticks? custom-y-axis-ticks?)
currency-symbol format-style custom-x-axis-ticks? custom-y-axis-ticks?)
html-chart?
(width html-chart-width html-chart-set-width)
(height html-chart-height html-chart-set-height)
(chart-options html-chart-chart-options html-chart-set-chart-options)
(currency-iso html-chart-currency-iso html-chart-set-currency-iso)
(currency-symbol html-chart-currency-symbol html-chart-set-currency-symbol)
(format-style html-chart-format-style html-chart-set-format-style)
(custom-x-axis-ticks? html-chart-custom-x-axis-ticks?
html-chart-set-custom-x-axis-ticks?)
(custom-y-axis-ticks? html-chart-custom-y-axis-ticks?
Expand All @@ -166,6 +169,8 @@
(define gnc:html-chart-set-currency-iso! html-chart-set-currency-iso)
(define gnc:html-chart-currency-symbol html-chart-currency-symbol)
(define gnc:html-chart-set-currency-symbol! html-chart-set-currency-symbol)
(define gnc:html-chart-format-style html-chart-format-style)
(define gnc:html-chart-set-format-style! html-chart-set-format-style)
(define gnc:html-chart-custom-x-axis-ticks? html-chart-custom-x-axis-ticks?)
(define gnc:html-chart-set-custom-x-axis-ticks?! html-chart-set-custom-x-axis-ticks?)
(define gnc:html-chart-custom-y-axis-ticks? html-chart-custom-y-axis-ticks?)
Expand Down Expand Up @@ -254,6 +259,7 @@
(cons 'text ""))))))
"XXX" ;currency-iso
"\u00A4" ;currency-symbol
"currency";format-style
#t ;custom x-axis ticks?
#t ;custom y-axis ticks?
))
Expand Down Expand Up @@ -341,10 +347,10 @@
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toLocaleString
var toLocaleStringSupportsOptions = (typeof Intl == 'object' && Intl && typeof Intl.NumberFormat == 'function');

// format a number e.g. 2.5 into monetary e.g. \"$2.50\"
// format a number e.g. 2.5 into monetary e.g. \"$2.50\" or other style formsty
function numformat(amount) {
if (toLocaleStringSupportsOptions) {
return amount.toLocaleString(undefined, {style:'currency', currency:curriso});
return amount.toLocaleString(undefined, {style:formsty, currency:curriso});
} else {
return currsym + amount.toLocaleString();
}
Expand Down Expand Up @@ -457,6 +463,7 @@ document.getElementById(chartid).onclick = function(evt) {
(push (format #f "<script id='script-~a'>\n" id))
(push (format #f "var curriso = ~s;\n" (gnc:html-chart-currency-iso chart)))
(push (format #f "var currsym = ~s;\n" (gnc:html-chart-currency-symbol chart)))
(push (format #f "var formsty = ~s;\n" (gnc:html-chart-format-style chart)))
(push (format #f "var chartid = ~s;\n" id))
(push (format #f "var jumpid = 'jump-~a';\n" id))
(push (format #f "var loadstring = ~s;\n" (G_ "Load")))
Expand Down
44 changes: 35 additions & 9 deletions gnucash/report/reports/standard/category-barchart.scm
View file
Open in desktop
Expand Up @@ -34,6 +34,12 @@
(use-modules (gnucash app-utils))
(use-modules (gnucash report))

;; useful functions
(define (safe-/ x y)
(if (and (zero? x) (zero? y)) 0 (/ x y)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(define (l1norm . numbers)
(fold (lambda (a b) (+ (abs a) b)) 0 numbers))

;; The option names are defined here to 1. save typing and 2. avoid
;; spelling errors. The *reportnames* are defined here (and not only
;; once at the very end) because I need them to define the "other"
Expand Down Expand Up @@ -175,6 +181,13 @@ developing over time"))
(N_ "Show table")
"e" (N_ "Display a table of the selected data.")
#f))

(add-option
(gnc:make-simple-boolean-option
gnc:pagename-display
(N_ "Replace amounts with percentage ratios.")
"e1" (N_ "Display percentage contribution of each account to the Gand Total instead of amounts.")
#f))

(gnc:options-add-plot-size!
options gnc:pagename-display
Expand Down Expand Up @@ -246,6 +259,7 @@ developing over time"))
(work-to-do 0)
(all-data #f)
(show-table? (get-option gnc:pagename-display (N_ "Show table")))
(ratio-chart? (get-option gnc:pagename-display (N_ "Replace amounts with percentage ratios.")))
(document (gnc:make-html-document))
(chart (gnc:make-html-chart))
(topl-accounts (gnc:filter-accountlist-type
Expand Down Expand Up @@ -494,7 +508,10 @@ developing over time"))
(let* ((dates-list (if do-intervals?
(list-head dates-list (1- (length dates-list)))
dates-list))
(date-string-list (map qof-print-date dates-list)))
(date-string-list (map qof-print-date dates-list))

; total amount with l1norm, so that assest and debts add up
(grandts (map (lambda (row) (apply l1norm (map gnc:gnc-monetary-amount row))) (apply zip (map cadr all-data)))))
christopherlam marked this conversation as resolved.
Show resolved Hide resolved

;; Set chart title, subtitle etc.
(gnc:html-chart-set-type!
Expand All @@ -514,7 +531,7 @@ developing over time"))

(gnc:html-chart-set-data-labels! chart date-string-list)
(gnc:html-chart-set-y-axis-label!
chart (gnc-commodity-get-mnemonic report-currency))
chart (if ratio-chart? "Ratio" (gnc-commodity-get-mnemonic report-currency)))

;; If we have too many categories, we sum them into a new
;; 'other' category and add a link to a new report with just
Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

(stack (if stacked? "default" (number->string stack)))
(fill (eq? chart-type 'barchart))
(urls (cond
Expand All @@ -578,7 +596,7 @@ developing over time"))
(gnc-account-get-full-name acct)
(xaccAccountGetName acct)))))))))
(gnc:html-chart-add-data-series!
chart label amounts color
chart label (if ratio-chart? percentage amounts) color
'stack stack 'fill fill 'urls urls)))
all-data
(gnc:assign-colors (length all-data))
Expand All @@ -589,6 +607,8 @@ developing over time"))
chart (gnc-commodity-get-mnemonic report-currency))
(gnc:html-chart-set-currency-symbol!
chart (gnc-commodity-get-nice-symbol report-currency))
(gnc:html-chart-set-format-style!
chart (if ratio-chart? "percent" "currency"))

(gnc:report-percent-done 98)
(gnc:html-document-add-object! document chart)
Expand All @@ -600,17 +620,23 @@ developing over time"))

(define (make-cell contents)
(gnc:make-html-table-cell/markup "number-cell" contents))

(define (make-cell-percent contents)
(gnc:make-html-table-cell/markup "number-cell" contents " %"))

(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)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


Expand Down