ChrisCScott / forecaster

A personal finances forecasting tool for Canadian retirement planning
Other
1 stars 2 forks source link

Tree-based transaction strategies #67

Closed ChrisCScott closed 5 years ago

ChrisCScott commented 5 years ago

This PR reimplements the former AccountTransactionStrategy as TransactionTraversal. The new class is radically more generic - it accepts a quasi-tree structure to define the flows of transactions to accounts. A new TransactionStrategy class wraps TransactionTraversal and mimics the interface of AccountTransactionStrategy. (Crucially, it's easy to build from a JSON settings file, unlike TransactionTraversal, which requires references to Account objects in memory.)

Another significant change is the introduction of LinkedLimitAccount in place of the former ContributionLimitAccount. This genericizes the concepts of the old class. For example, a LinkedLimitAccount can share limits on any combination of min/max inflows/outflows - and can share limits for each of those with different sets of linked accounts. It no longer uses a Person object to link those accounts, instead offloading this to an AccountLink object which takes an arbitrary token (which may be a Person object, if that's what binds common accounts together).

This has necessitated some changes to RegisteredAccount (in the canada submodule) and to recorded_property (to support setters and deleters). RRSP and TFSA did not require any changes on this basis, although they do receive some unrelated changes.

Some basic Account-level interfaces have been changed, necessitating changes throughout the package. outflows and inflows are now methods (rather than properties), to better support optionally passing in not-yet-recorded transactions. (In later revisions we might offload these to a new Transactions class). Other methods, like min_inflows and similar, now accept similar arguments as well, and existing arguments have been reordered.

Debt is simplified. It now acts more like a regular account (with negative balance); the concept of payments from savings vs. from living expenses has been offloaded to higher-level code. It never really belonged with the Debt class anyways. It can be relatively easily represented by TransactionTraversal.

A number of files are now directories (or, in Python-speak, some modules are now packages), requiring changes to and additions of various __init.py__ files. Several util.py files have been added and various former methods have been changed to functions and moved there. The top-level utility.py needs to be revised to (a) conform to this naming scheme and (b) carve out all Timing-related code to a dedicated module.

TestTransactions is a new custom test case which provides the assertTransactions method. This is a convenience method for testing the (approximate) value of transaction dicts, either against other dicts or against scalar values. It makes life a lot easier.

Along the way, many minor improvements have been made as tests have been tightened up, expanded, or redesigned. Most SubForecast subclasses are now cleaner and less error-prone than before. RRSP has been tweaked to avoid converting to an RRIF earlier than required unless specifically told to do so. LinkedLimitAccount objects now advance each other automatically and avoid duplicating effort when updating their shared limits. Lots of new and expanded doctrings. It's safe to say that the current code is cleaner, better-documented, more generic, less buggy, and more powerful than before.

There's lots left to do. I've come across an actual computer science problem - TransactionTraversal is built around tree traversal, but due to duplication of nodes and/or the presence of LinkedLimitAccount objects it's not actually a proper tree. Issue #66 considers how to achieve the desired behaviour using graph-based techniques (e.g. network flow algorithms). Addition of a Transactions class (similar to the Timing class - likely a subclass of dict) is outstanding; such a class could add inflows, outflows, and other methods/properties (total?), along with potentially arithmetic operations. Consider whether it should be immutable (probably not?). The use of per-node limits in TransactionTraversal needs to be better-tested, especially when also dealing with LinkedLimitAccount nodes. And newly-added floating-point comparison constants (i.e. EPSILON) needs to be used more widely. We've been slack on updating the issue tracker with many of these - ensuring that all issues are appropriately tracked should be issue #0.