ChrisCScott / forecaster

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

Make *Strategy classes handle multiple timings/sources #51

Closed ChrisCScott closed 5 years ago

ChrisCScott commented 6 years ago

Forecast now has considerable logic in its gross-contribution-determining code, and will soon have similar logic in its contribution-reduction code. It also requires a workaround for current behaviour of ContributionStrategy and will likely require revisions to ContributionTransactionStrategy (which is entirely based on lump-sum contributions).

This is a set of ugly hacks which are likely to break easily. It could be improved simply by allowing ContributionStrategy to return a dict of source: value pairs (or even timing: value pairs?) and allowing ContributionTransactionStrategy to ingest a dict of timing: value pairs instead of using its current timing attribute. This will involve changing the call signatures of the classes in other ways; for example, right now ContributionStrategy receives a scalar for net_income, but perhaps it instead needs to receive a set of Person objects. Consider whether WithdrawalStrategy will require the same or different tweaks.

TransactionStrategy may need to be revised to be callable multiple times with different (scalar) timings, or once with more complex timing: value dict as input. It's likely that each tranche of in/outflows will need to be added to the relevant Account object before calling the next tranche. Same goes for DebtPaymentStrategy (which therefore should not automatically assign the full minimum payments when there's not sufficient money!)

The benefit to this is that we can migrate some of the more complex logic out of Forecast and contain it in the more appropriate classes, at the cost of potentially making the call signature of those classes more complex.

ChrisCScott commented 6 years ago

Note that this is closely related to issue #49.

ChrisCScott commented 5 years ago

Following resolution of issues #40 and #49, this issue's resolution will look a little different. There is no longer a ContributionStrategy class. Most strategies relevant to Forecaster are AccountTransactionStrategy-like, in that they emit {Account: value} pairs. (LivingExpensesStrategy emits a scalar value, since there is no set of accounts to map to). So multiple sources are adequately handled by the revised codebase.

However, all of these strategy classes ingest a scalar total_available argument and return timing-free values. Consider whether we want to be able to pass in available (or perhaps some derived dict that merges in/outflows to provide a dict of only positive values available for use at any given time) and receive a mapping of the form {Account: {timing: value}}. This happens to be the structure of the TransactionDict class; perhaps this should be moved to Utility if it is to also be used by Strategy?

That's an option, but it's not clear what benefit it provides - the current functionality leave timing logic to Forecaster, which is adequate. Propose closing and retaining current functionality.

ChrisCScott commented 5 years ago

After some review, it appears that handling timing information is important at least in DebtPaymentStrategy (and likely also in AccountTransactionStrategy for similar reasons) at least because determining how much can be contributed to a debt is a function of when payments are made (due to interest).

The existing timing logic in this class is insufficient - DebtPaymentStrategy had a scalar timing attribute which it used for determining maximum inflows or outflows. Since we generally assign inflows/outflows based on each debt's payment_timing attribute (which includes multiple timings), this is likely to result in inconsistent behaviour.

Consider adding max_inflows and max_outflows methods to Debt (or maybe even to Account) and let it take a Timing dict as input. It would then need to return a dict of transactions (or possibly a scalar value such that {timing: weight * value for timing, weight in timings.items()} yields that dict). The Strategy classes could then relatively straightforwardly receive timing dicts and pass them along as needed. We could optionally add timing as an attribute to Account, although it makes more sense to allow dynamic timing information to be passed in from Forecaster (via available).