Introduce multi-period Account data type and use it for MultiBalanceReport and BudgetReport. #2360

Xitian9 · 2025-03-26T12:09:02Z

This rejigs the MultiBalanceReport internals to use an enhanced Account data type to save the values. This has a few effects:

A small speed improvement on large journal files with interval reporting, due to processing the posting list in one pass.
Simplified program logic, as there was a lot of code to convert back and forth between list and tree representations. This can now be removed.
Ability to merge Account means that BudgetReport can be simplified.

There are some small changes in behaviour with respect to budget reports, where it looked like some behaviour was implemented to work around needing to get the budget and actuals into the same shape so they could be merged. This is no longer necessary, but may still be desired for other reasons.

Let me know your thoughts.

This upgrades Account to enable it to do the hard work in MultiBalanceReport, but does not use the new functionality just yet. It continues to function as before by only using the "abhistorical" value.

…and Account.

For use in budget reports.

Ensure that implied accounts with no postings are not displayed, but accounts with zero balance and actual postings are.

Rephrase everything in terms of boringness to make for a clearer logical flow.

This removes the type alias Account, and replaces it with the fully-qualified name Account AccountBalance. This breaks some backwards compatibility, but that was already broken by the change of Account type constructor in any case. This simplifies the interface.

mergeWithKey can create corrupt output if its inputs don't satisfy certain conditions. We restrict the domain here to only those cases where it is guaranteed safe. This still covers all the cases that we need.

simonmichael

Thanks! Initial comments.

simonmichael · 2025-04-03T18:05:59Z

hledger-lib/Hledger/Data/AccountBalance.hs

+
+-- tests
+
+tests_AccountBalance = testGroup "AccountBalance" [


Keep these separate for clarity if you like, but I suggest grouping them under tests_AccountBalance for export to avoid breaking the consistent pattern (every module exports one similarly named test group)

simonmichael · 2025-04-03T18:07:24Z

hledger-ui/hledger-ui.cabal

@@ -9,7 +9,6 @@ version:        1.42.99
 synopsis:       Terminal interface for the hledger accounting system
 description:    A simple terminal user interface for the hledger accounting system.
                It can be a more convenient way to browse your accounts than the CLI.
-                This package currently does not support Microsoft Windows, except in WSL.
                .


This change is in master and will disappear after rebase I think.

simonmichael · 2025-04-03T18:09:08Z

hledger/test/balance/balance.test

-account a:ab
-account a:ac:aca
-account b
+# ** 11. In flat mode we can see that undeclared accounts are excluded.


Text trivia:

I have been trying to move in the direction of "list" actually as the preferred term actually (for the -l short flag, eg. Possibly should go all the way and add --list long flag ?)

But your change matches the flag used in the command, so maybe it's better..

I feel maybe the old test description here was bad.. I think --declared includes declared accounts, rather than excluding undeclared ones. Perhaps we should drop that text.

I'm ok with the behaviour change here personally. I'd put a ! (= "breaking change") after the first commit message prefix which helps me notice these at changelog/relnotes time.

ref: is a new prefix meaning "refactoring" I guess. Ok, but perhaps it isn't the right one here since this is not strictly a refactoring. In such cases I often go with a generic dev:.

simonmichael · 2025-04-03T18:28:24Z

hledger/test/balance/balance.test

+
+# ** 16. balance --flat --empty does not display accounts which have not been
+# seen, even if they're implied, but does show accounts that have been seen
+# with 0 balance.


I'm not sure what this means in userese ?

simonmichael · 2025-04-03T18:33:41Z

hledger/test/balance/budget.test

@@ -50,7 +50,6 @@ Budget performance in 2016-12-01..2016-12-03:
                  ||          2016-12-01          2016-12-02           2016-12-03 
 ==================++==============================================================
 assets:cash      || $-10 [ 40% of $-25]  $-14 [56% of $-25]  $-51 [204% of $-25] 
- expenses         ||  $10 [ 40% of  $25]   $14 [56% of  $25]   $51 [204% of  $25] 


Another behaviour change, worth noting with a ! in message.

The parent "expenses" account is not shown, because there's no explicit budget goal for it, and because we're in list mode ? So if we want to see aggregated budget performance, tree mode will be needed. Ok I guess.

Why is it still shown in the previous test ?

simonmichael · 2025-04-03T18:37:02Z

hledger/test/balance/budget.test

@@ -747,8 +745,7 @@ Budget performance in 2023-08:
 ==============++=================
 income       || 0 [0% of $-200] 
   employment || 0 [0% of $-100] 
-   gifts      || 0               
-     cash     || 0 [0% of $-100] 
+   gifts:cash || 0 [0% of $-100] 


This looks like a consistency fix, squashing these like a normal balance report would. I think I tried and failed to fix this recently. 👍🏻

simonmichael · 2025-04-03T18:37:42Z

hledger/test/incomestatement.test

@@ -296,6 +296,7 @@ Income Statement ..
 =================++====
 Revenues        ||    
 -----------------++----
+ revenues        ||  0 


Should drop "leaf" from this test's description now I guess.

simonmichael · 2025-04-03T18:44:43Z

hledger-lib/Hledger/Data/Types.hs

+data AccountBalances a = AccountBalances {
+   abhistorical :: a            -- ^ historical balance information
+  ,abdatemap    :: IM.IntMap a  -- ^ balance information associated to a start day
+  } deriving (Eq, Functor, Generic)


What's the reason for using a type parameter ? Do we truly need it ?

How does the "start day" map, with Int keys I assume, work here ?

What's the reason for using a type parameter ? Do we truly need it ?

For budget report, I guess. It hurts code comprehensability a bit.

simonmichael · 2025-04-03T18:47:51Z

hledger-lib/Hledger/Data/Types.hs

@@ -733,18 +735,31 @@ nullaccountdeclarationinfo = AccountDeclarationInfo {

 -- | An account, with its balances, parent/subaccount relationships, etc.


Perhaps we should change this to "An account within a hierarchy", or something. Representing/navigating a hierarchy of account balances is Account's purpose.

simonmichael · 2025-04-03T19:01:24Z

A small speed improvement on large journal files

Could we quantify that a little more - eg "balance reports are 1% faster with 1k txns, 5% faster with 10k txns" ?

simonmichael · 2025-04-03T19:02:19Z

Also I wonder if there's any memory impact, stats might be a quick way to check (it runs balance reports probably)

simonmichael · 2025-04-03T19:08:20Z

hledger-lib/Hledger/Data/AccountBalance.hs

+instance Monoid a => Monoid (AccountBalances a) where
+  mempty = AccountBalances mempty mempty
+
+-- | Construct an 'AccountBalance' from a list.


Construct an 'AccountBalances' from a list

simonmichael · 2025-04-03T19:17:11Z

hledger-lib/Hledger/Data/AccountBalance.hs

+accountBalancesFromList :: a -> [(Day, a)] -> AccountBalances a
+accountBalancesFromList h = AccountBalances h . IM.fromList . map (\(d, a) -> (fromInteger $ toModifiedJulianDay d, a))
+
+-- | Get the 'AccountBalance' associated to the period containing a given 'Day'.


More general doc: Get the account balance information associated ... ?

simonmichael · 2025-04-03T19:17:40Z

hledger-lib/Hledger/Data/AccountBalance.hs

+lookupAccountBalance d (AccountBalances h as) =
+    maybe h snd $ IM.lookupLE (fromInteger $ toModifiedJulianDay d) as
+
+-- | Add the 'AccountBalance' to the appropriate location in 'AccountBalances'.


Add balance information ...

simonmichael · 2025-04-03T19:21:03Z

hledger-lib/Hledger/Data/AccountBalance.hs

+{-|
+
+
+An 'AccountBalance' consists of a historical balance, along with balances


Maybe clarify that this module contains operations for two distinct types: AccountBalances and AccountBalance. (Easy for a reader to mix up). Almost worth separate modules ?

simonmichael · 2025-04-03T19:25:46Z

hledger-lib/Hledger/Data/AccountBalance.hs

+    Nothing  -> balances{abhistorical = abhistorical balances <> b}
+    Just day -> balances{abdatemap = IM.insertWith (<>) (fromInteger $ toModifiedJulianDay day) b $ abdatemap balances}
+
+-- | Performs an operation on the contents of two 'AccountBalances'.


Merges two AccountBalances, using the given operation to combine their balance informations. ?

simonmichael · 2025-04-03T19:27:19Z

hledger-lib/Hledger/Data/AccountBalance.hs

+
+-- | Performs an operation on the contents of two 'AccountBalances', with
+-- separate treatments for those only in the first, only in the second, or in
+-- both 'AccountBalance's.


Merges two AccountBalances, with separate operations for balance informations only in ... ?

simonmichael · 2025-04-03T19:29:03Z

hledger-lib/Hledger/Data/AccountBalance.hs

+applyAccountBalance :: (MixedAmount -> MixedAmount) -> AccountBalance -> AccountBalance
+applyAccountBalance f a = a{abebalance = f $ abebalance a, abibalance = f $ abibalance a}
+
+-- | Perform an operation on the 'MixedAmount' in two 'AccountBalance'.


Merge two AccountBalance, using the given operation to combine their amounts ?

simonmichael · 2025-04-03T19:31:14Z

hledger-lib/Hledger/Data/Types.hs

-  ,aebalance                 :: MixedAmount    -- ^ this account's balance, excluding subaccounts
-  ,aibalance                 :: MixedAmount    -- ^ this account's balance, including subaccounts
-  } deriving (Generic)
+  ,abalances                 :: AccountBalances a   -- ^ historical and date-associated account balances


It's hard to be clear on this everywhere, but maybe here mention both "end balances" (on a date) and "balance changes" (in a period) (consistent with terminology elsewhere) are/may be being represented by "AccountBalance(s)".

simonmichael · 2025-04-03T19:44:35Z

hledger-lib/Hledger/Data/Account.hs

-        aname=a
-       ,asubs=map (uncurry accountTree') $ M.assocs m'
-       }
+      (accountFromBalances a bals){ asubs=map (uncurry accountTree') $ M.assocs m' }


This doc and function is a bit hard to understand, any clarification welcome

simonmichael · 2025-04-03T19:46:33Z

hledger-lib/Hledger/Data/Account.hs

 mapAccounts f a = f a{asubs = map (mapAccounts f) $ asubs a}

+-- | Apply a function to all 'AccountBalances' within this and sub accounts.


Similar functions in AccountBalances.hs are named "apply" - maybe "map" is best, like this one ?

simonmichael · 2025-04-03T19:49:54Z

hledger-lib/Hledger/Data/Account.hs

+allAccounts p a
+    | not (p a) = False
+    | otherwise = all (allAccounts p) $ asubs a
+
 -- | Add subaccount-inclusive balances to an account tree.


While you're here, we could make this one clearer:
Recalculate all the subaccount-inclusive balances in this account tree.

simonmichael · 2025-04-03T19:50:55Z

hledger-lib/Hledger/Data/Account.hs

+    subtotals = foldMap abalances subs
+
+    setImplicitBalances :: AccountBalances AccountBalance -> AccountBalances AccountBalance
+    setImplicitBalances = mergeAccountBalances onlyChildren noChildren combineChildren subtotals


Did you mean setInclusiveBalances ?

simonmichael · 2025-04-03T19:53:11Z

hledger-lib/Hledger/Data/Account.hs

-- sort first, and vice versa.
-sortAccountTreeByAmount :: NormalSign -> Account -> Account
-sortAccountTreeByAmount normalsign = mapAccounts $ \a -> a{asubs=sortSubs $ asubs a}
+-- | Merge two accounts and their subaccounts.


Merge two account trees ?

simonmichael · 2025-04-03T20:14:24Z

hledger-lib/Hledger/Data/Account.hs

+-- | Derive 1. an account tree and 2. each account's total exclusive
+-- and inclusive changes from a list of postings.
+-- This is the core of the balance command (and of *ledger).
+-- The accounts are returned as tree.


simonmichael · 2025-04-03T20:21:14Z

hledger-lib/Hledger/Data/Account.hs

@@ -85,33 +108,47 @@ nullacct = Account
 -- The accounts are returned as a list in flattened tree order,
 -- and also reference each other as a tree.
 -- (The first account is the root of the tree.)


In this (and maybe the next), let's explain a bit more that this calculates multiple inclusive and exclusive change amounts for each account, associated with the dates produced by the given date-bucketing function (usually representing the end dates of report subperiods)

(It looks like this will enable non-contiguous report periods in future, which will be cool)

Xitian9 · 2025-04-05T00:47:47Z

Thanks for the detailed comments. I'm on holiday at the moment, so it'll be about 3 weeks before I can make the requested changes, but I will when I get back.

Xitian9 added 15 commits March 26, 2025 23:31

ref: lib: Allow Account to store date-indexed balances.

7dbcae1

This upgrades Account to enable it to do the hard work in MultiBalanceReport, but does not use the new functionality just yet. It continues to function as before by only using the "abhistorical" value.

ref: lib: Allow generating day-indexed Account from postings.

d1b5407

ref: lib: Better show instances for AccountBalance, AccountBalances, …

f1b45d3

…and Account.

imp: lib: Create mergeAccounts to merge two Accounts.

55df2ff

For use in budget reports.

test: Add new test for --flat --empty

38429e8

Ensure that implied accounts with no postings are not displayed, but accounts with zero balance and actual postings are.

test: Make --declared treat parent accounts consistently

ce2f561

ref: lib: Begin using new Account for MultiBalanceReport.

6d314a5

fix: lib: Ensure accounts with no postings are not shown in flat mode.

f9dc496

fix: Make sure declared accounts are added.

35a4ba5

ref: Allow more efficient and general calculation of PeriodicReportRows.

547d8a1

ref: Sorting Account by amount needs to specify which amount.

072ead6

ref: lib: Use new infrastructure in budget report.

e7efffe

ref: lib: Refactor boolean logic in markAccountBoring.

2fd2fee

Rephrase everything in terms of boringness to make for a clearer logical flow.

fix: Get hledger-ui and hledger-web working with new Account interface.

e23ea50

Xitian9 force-pushed the multiaccount branch 3 times, most recently from 82a01da to 6745f0a Compare March 27, 2025 12:54

ref: lib: Use sparse accounts to increase efficiency and flexibility.

31d4d2c

Xitian9 force-pushed the multiaccount branch from 6745f0a to 31d4d2c Compare March 28, 2025 09:33

Xitian9 added 2 commits March 28, 2025 20:46

ref: lib: Clean up unused functions and add mapAccountBalances.

898b1d0

ref: lib: Make mergeAccountBalances safer.

04cb729

mergeWithKey can create corrupt output if its inputs don't satisfy certain conditions. We restrict the domain here to only those cases where it is guaranteed safe. This still covers all the cases that we need.

Xitian9 force-pushed the multiaccount branch from d8d6312 to 04cb729 Compare March 29, 2025 11:47

simonmichael added needs-review To unblock: needs more code/docs/design review by someone performance Anything performance-related (run time, memory usage, disk space..) labels Apr 2, 2025

simonmichael reviewed Apr 3, 2025

View reviewed changes

simonmichael added balance balancesheet incomestatement and removed needs-review To unblock: needs more code/docs/design review by someone labels Apr 3, 2025

simonmichael added cashflow budget The balance command's --budget report balancesheetequity labels Apr 3, 2025

simonmichael reviewed Apr 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce multi-period Account data type and use it for MultiBalanceReport and BudgetReport. #2360

Introduce multi-period Account data type and use it for MultiBalanceReport and BudgetReport. #2360

Xitian9 commented Mar 26, 2025

simonmichael left a comment •

edited

Loading

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025

simonmichael commented Apr 3, 2025

simonmichael commented Apr 3, 2025

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025

simonmichael Apr 3, 2025 •

edited

Loading

Xitian9 commented Apr 5, 2025 via email •

edited by simonmichael

Loading

		@@ -733,18 +735,31 @@ nullaccountdeclarationinfo = AccountDeclarationInfo {

		-- \| An account, with its balances, parent/subaccount relationships, etc.

		{-\|


		An 'AccountBalance' consists of a historical balance, along with balances

		mapAccounts f a = f a{asubs = map (mapAccounts f) $ asubs a}

		-- \| Apply a function to all 'AccountBalances' within this and sub accounts.

Introduce multi-period Account data type and use it for MultiBalanceReport and BudgetReport. #2360

Are you sure you want to change the base?

Introduce multi-period Account data type and use it for MultiBalanceReport and BudgetReport. #2360

Conversation

Xitian9 commented Mar 26, 2025

simonmichael left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonmichael Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonmichael Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonmichael commented Apr 3, 2025

simonmichael commented Apr 3, 2025

Choose a reason for hiding this comment

simonmichael Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonmichael Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

simonmichael Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

simonmichael Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonmichael Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonmichael Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

simonmichael Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

simonmichael Apr 3, 2025 • edited Loading

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

simonmichael Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Xitian9 commented Apr 5, 2025 via email • edited by simonmichael Loading

simonmichael left a comment •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

simonmichael Apr 3, 2025 •

edited

Loading

Xitian9 commented Apr 5, 2025 via email •

edited by simonmichael

Loading