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.

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

Pie chart #1467

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Pie chart #1467

wants to merge 7 commits into from

Conversation

raboof
Copy link
Contributor

@raboof raboof commented Jan 19, 2021

@raboof raboof mentioned this pull request Jan 19, 2021
@simonmichael
Copy link
Owner

Thanks for reviving this!

make help-buildtest shows some make rules that may help show errors like the CI does.

@simonmichael simonmichael added A-WISH Some kind of improvement request, hare-brained proposal, or plea. web The hledger-web tool. labels Jan 22, 2021
@simonmichael
Copy link
Owner

Also, screenshots welcome when you get to that point!

@raboof
Copy link
Contributor Author

raboof commented Jan 22, 2021

Yes, planning to get back to fixing the warnings and adding some more things later ;). Pretty basic for now, but already on par with the existing work I think:

hledger

(snipped out most data for privacy - is there a '3DBenchy for hledger' canonical test dataset somewhere bychance?)

@simonmichael
Copy link
Owner

Nifty. Is it in addition to/instead of the line chart ?

examples/bcexample.hledger is a pretty good example journal.

@raboof
Copy link
Contributor Author

raboof commented Jan 24, 2021

That's a lot of taxes! (it's looks like it is showing the USD and the IRAUSD taxes as separate slices, also haven't checked those are converted correctly so the % values make sense)

pie

@raboof raboof marked this pull request as ready for review January 25, 2021 21:23
@raboof
Copy link
Contributor Author

raboof commented Jan 26, 2021

showing 2 pie charts when there are both positive and negative transactions might still be a good improvement, but I think this PR is ready to be reviewed even without that.

@simonmichael
Copy link
Owner

showing 2 pie charts when there are both positive and negative transactions might still be a good improvement, but I think this PR is ready to be reviewed even without that.

(Thought I replied to this earlier, but I don't see it.) @raboof, won't it potentially show misleading results without that capability ? I think we need it.

@raboof raboof marked this pull request as draft January 29, 2021 08:30
@raboof
Copy link
Contributor Author

raboof commented Jan 29, 2021

won't it potentially show misleading results without that capability ? I think we need it.

Updated to split the pie chart into positive and negative accounts (only showing them if there is more than one account to show).

The chart would still be confusing when accounts use multiple currencies. I don't think splitting those into multiple pie charts as well would make too much sense - I'm just hiding the pie charts in that case for now.

@raboof raboof marked this pull request as ready for review January 29, 2021 11:00
@simonmichael
Copy link
Owner

That sounds good. Will review as soon as I clear some PR backlog, thank you! Other reviewers welcome.

Copy link
Owner

@simonmichael simonmichael left a comment

Choose a reason for hiding this comment

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

@raboof: I've got this running locally. Great work so far. I'd like to improve usability a bit more if you have time. Do you think some of these could be done:

  • Always show both charts, in the same stable positions. When one can't be shown for whatever reason, show it greyed out, or at least some placeholder.
  • Show the charts side by side, to use less vertical space. Negative amounts always on the left, positives on the right I guess.
  • In the legend, show the amounts. I know you'll be short of space, maybe the percentage is not necessary there ?

@@ -103,6 +103,7 @@ extra-source-files:
templates/edit-form.hamlet
templates/journal.hamlet
templates/manage.hamlet
templates/piechart.hamlet
Copy link
Owner

@simonmichael simonmichael Feb 2, 2021

Choose a reason for hiding this comment

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

Would you mind making this same change in the package.yaml file, from which this is generated. (Changing the .cabal file as well is optional, stack does it automatically and I sometimes commit that change separately/later to reduce conflicts.)

@simonmichael
Copy link
Owner

simonmichael commented Feb 2, 2021

Also, I have seen it show two pies for one particular account, but most of the time it shows only one, or none. Perhaps there's a bug ? Eg with examples/sample.journal, a single pie is shown for expenses and for income, and no pies for any other account.

@raboof
Copy link
Contributor Author

raboof commented Feb 2, 2021

I'd like to improve usability a bit more (...) Always show both charts, in the same stable positions. When one can't be shown for whatever reason, show it greyed out, or at least some placeholder

Funny, I introduced hiding charts when there is no meaningful data a usability feature, not a shortcoming :D. Happy to adapt though.

I have seen it show two pies for one particular account, but most of the time it shows only one, or none. Perhaps there's a bug ? Eg with examples/sample.journal, a single pie is shown for expenses and for income, and no pies for any other account.

It shows a pie chart when there is more than one (sub)account being reported on, unless those (sub)accounts contain multiple commodities. That was intentional, but if it creates more confusion than it reduces noise then we should indeed implement a more straightforward behaviour ;).

@simonmichael
Copy link
Owner

simonmichael commented Feb 2, 2021 via email

@raboof
Copy link
Contributor Author

raboof commented Feb 2, 2021

Eg with examples/sample.journal, a single pie is shown for expenses and for income, and no pies for any other account.

This matches what I would expect:

  • expenses has 2 subaccounts with a positive balance, so those are shown in a pie chart
  • income has 2 subaccounts with a negative balance, so those are shown in a pie chart
  • 'assets' has 2 subaccoutns: 1 with negative balance and 1 with positive balance. Since showing a pie chart with just one '100%' category didn't feel very useful to me, I wasn't showing those.
  • all other accounts don't have subaccounts, so don't have pie charts

So this was indeed my intended behavior - but evidently that wasn't very intuitive. I'll look into always showing both charts.

@simonmichael
Copy link
Owner

simonmichael commented Feb 2, 2021 via email

@simonmichael
Copy link
Owner

Hi @raboof, I just tested your latest push.

The solid pie charts are a bit clunky of course, but for now, they do reassure that the feature is working..

Any idea why no charts are shown for the sample journal's assets:bank:checking ? http://127.0.0.1:5000/register?q=inacct%3Aassets%3Abank%3Achecking%20

Wouldn't it be more familiar to have the chart for positives on the right, and negatives on the left ?

I wonder how it feels if you show amounts instead of percentages ?

Do you think it's feasible to have some useful info (eg amount) always showing on or near the pie slices themselves ?

@Xitian9
Copy link
Collaborator

Xitian9 commented Feb 12, 2021

I hate to be “that person”, especially when so much great work has been put in here, but I feel compelled to raise the point that it's notoriously difficult for people to be able to extract accurate information from pie charts.

When I generate my financial charts, I use a stacked bar chart with absolute amounts on the left y-axis and percentage of total on the right y-axis for this purpose. It's easier to read, easier to display, and allows you to include both positive and negative amounts in a sensible way.

@raboof
Copy link
Contributor Author

raboof commented Feb 12, 2021

Any idea why no charts are shown for the sample journal's assets:bank:checking?

(will check when I get back to this)

Wouldn't it be more familiar to have the chart for positives on the right, and negatives on the left?

Sounds reasonable, will update.

I wonder how it feels if you show amounts instead of percentages ? Do you think it's feasible to have some useful info (eg amount) always showing on or near the pie slices themselves ?

I think that would be a great idea (though there's something to be said about percentages as well, as they are less likely to invoke 'fear of big numbers' :D ). Perhaps not absolutely necessary for this first iteration though - haven't looked into how hard it would be yet.

I hate to be “that person”, especially when so much great work has been put in here

Don't worry ;)

I feel compelled to raise the point that it's notoriously difficult for people to be able to extract accurate information from pie charts.

To be honest I feel like many of those points don't apply so strongly here. Obviously the '3d' pie charts are pure evil. We don't really seem to suffer from any of the '4 signs that your pie is failing', mainly because it's interactive rather than a static image. The 'small percentages' aren't particularly important in our case.

When I generate my financial charts, I use a stacked bar chart with absolute amounts on the left y-axis and percentage of total on the right y-axis for this purpose. It's easier to read, easier to display, and allows you to include both positive and negative amounts in a sensible way.

I would love to have those, and they might indeed replace pie charts. Until we have those, though, I think at least having the pie charts would be a significant improvement over not having them :).

@simonmichael
Copy link
Owner

simonmichael commented Feb 17, 2021

Good discussion. @raboof stop me if the below seems not the latest code, I think it is though. I'm testing with
make ghci-web, :main -f examples/sample.journal. [edit: fixed typo]

  • I want to see amounts, it's hard to understand what these pies are showing otherwise
  • shows 0, 1 or 2 pies for each account, it's hard to understand why
  • still showing positives on left, negatives on right
  • click assets, move mouse pointer sideways across both pies; one of them is left in highlighted mouseover state
  • should this show the subaccounts within the displayed account, or the other accounts being transacted with ?
    Currently it shows the former, but I think people are going to struggle with this. Maybe it should show the other instead,
    or maybe there's some way to make it more obvious what's being shown.

Sorry this is hard to get merged. The UI polish is a big part of the task, and in the present state I'm not sure it adds enough value to release it.

@raboof raboof marked this pull request as draft February 17, 2021 16:00
@raboof
Copy link
Contributor Author

raboof commented Feb 17, 2021

stop me if the below seems not the latest code, I think it is though

Yeah it is - I should've converted the PR to 'draft' though since I didn't yet find time to pick up some of the comments that I do plan to.

Sorry this is hard to get merged.

No worries - I only find time to work on this occasionally so that rather drags things out wall-clock-wise, but so far I'm happy with the process.

@raboof
Copy link
Contributor Author

raboof commented Feb 17, 2021

should this show the subaccounts within the displayed account, or the other accounts being transacted with ?
Currently it shows the former, but I think people are going to struggle with this. Maybe it should show the other instead,
or maybe there's some way to make it more obvious what's being shown.

My use case is definitely 'subaccounts': I import and auto-classify my banking statements, and then use hledger-web to get a feel for 'where my money is going' (and later I'd like to add budgeting). The pies are nice for that while browsing the 'expenses' tree, like 'x % of my expenses in this period where utilities', with the option to from there 'zoom in' to the individual accounts under 'utilities', etc.

Showing charts of the 'accounts being transacted with' would also kinda work: I could open assets:bank and see what it transacts with. However, that would lose the hierarchy in the expenses. Also for more complicated journals it would seem browsing the 'expenses' tree hierarchically would make more sense than 'accounts being transacted with'.

@simonmichael
Copy link
Owner

simonmichael commented May 16, 2021 via email

@simonmichael
Copy link
Owner

simonmichael commented May 16, 2021 via email

@Xitian9
Copy link
Collaborator

Xitian9 commented May 16, 2021

Here is an overall breakdown of expenses and savings; the graph I use to take the place of a pie chart in showing proportion.
0-breakdown-0

A graph showing total cashflow in and out over the last year. Amounts are a simple moving average over the previous 14 days.
1-cashflow-0

Graph of net position, i.e. total assets minus total liabilities.
2-netposition-0

Graph of net liquid position.
3-liquid-0

Total assets and liabilities. Since I haven't bothered separating liquid from non-liquid assets and liabilities, this is similar to the previous graph.
4-wealth-0

The next three are graphs of expense categories over the past year, grouped together so the axes aren't too different.
5-expenses-0
5-expenses-1
5-expenses-2

@raboof
Copy link
Contributor Author

raboof commented May 16, 2021

there are a lot of different needs for charts and different aspects to making them usable and robust.

Sure!

I think we should do the R & D and prototyping outside of hledger-web where possible, eg using separate tools like ploterific

Can't hurt ;)

the new https://hledger.org/charting.html

(https://hledger.org/charts.html?)

When we see some charts that seem to have wide usefulness, we can tackle moving them into hledger-web as a separate task.

I'm personally mainly interested in 'interactively' browsing the data (selecting date ranges and subaccounts). Some graphs might not really be that useful when you have to switch between several tools to see them, but could be useful when integrated into hledger-web - so if we wait for them to "seem to have wide usefullness" outside of hledger-web they might never.

we can test and compare somewhat realistic examples, perhaps examples/bcexample.hledger will work as a standard data set

I agree a standard example might be helpful. A complication in this particular dataset is that it includes expenses in different currencies (USD and IRAUSD), making them trickier to graph of course - which might be an advantage or a disadvantage depending how you look at it.

Do you have ideas on how to handle multiple currencies in one graph?

Here is an overall breakdown of expenses and savings; the graph I use to take the place of a pie chart in showing proportion.

This is not bcexample.hledger, right? It's not clear to me why "Food" is split into ''Irregular expenses" and "Regular expenses", how does that work?

@Xitian9
Copy link
Collaborator

Xitian9 commented May 16, 2021

This is not bcexample.hledger, right? It's not clear to me why "Food" is split into ''Irregular expenses" and "Regular expenses", how does that work?

I classify groceries as regular expenses and restaurants and irregular expenses. I grant that this breakdown is specific to me, and it's probably not desirable to apply this widely.

@simonmichael simonmichael force-pushed the master branch 8 times, most recently from 56bc295 to 01f9c70 Compare July 11, 2021 09:26
@simonmichael
Copy link
Owner

What should we do with this PR ?

@raboof
Copy link
Contributor Author

raboof commented Nov 21, 2021

What should we do with this PR ?

It's up to you: I still like the feature and am happily using this branch of hledger. Of course I'd hoped it'd be of use to more people than just myself, but I respect that you don't consider it acceptable for inclusion in this state. I don't really see how to make it more attractive, though :). I'm OK with either closing the PR or keeping it open for future suggestions.

@simonmichael simonmichael added needs:clarification To unblock: needs to be simplified/clarified/explained/demoed needs:testing To unblock: needs more developer testing or general usage needs:changes To unblock: needs some changes made, in line with recommendations needs:rebase To unblock: needs to be rebased against latest master branch needs:design To unblock: needs more thought/planning, leading to a spec/plan labels Apr 6, 2023
didn't add a label since that would probably just be confusing,
which is which is likely pretty clear from context.
Since the result would be hard to understand anyway
No charts is shown when there is no data or the data is in multiple
commodities. Perhaps in that case we should show some error and/or
a 'greyed-out' chart?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-WISH Some kind of improvement request, hare-brained proposal, or plea. needs:changes To unblock: needs some changes made, in line with recommendations needs:clarification To unblock: needs to be simplified/clarified/explained/demoed needs:design To unblock: needs more thought/planning, leading to a spec/plan needs:rebase To unblock: needs to be rebased against latest master branch needs:testing To unblock: needs more developer testing or general usage web The hledger-web tool.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants