ChrisCScott / forecaster

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

Support optional high-precision numerical inputs #77

Closed ChrisCScott closed 2 years ago

ChrisCScott commented 2 years ago

Now that forecaster has no dependency on Decimal or Money (see #56), there are several new exceptions being raised when test suites provide Decimal inputs or inputs intended for Decimal conversion.

For instance, running accounts/test_base.py produces these errors:

TypeError: unsupported operand type(s) for -: 'decimal.Decimal' and 'float'

TypeError: unsupported operand type(s) for ** or pow(): 'float' and 'decimal.Decimal'

   self.assertTrue(type_check(account.rate_history, {int: Decimal}))
AssertionError: False is not true

    with self.assertRaises(decimal.InvalidOperation):
AssertionError: InvalidOperation not raised

AssertionError: {2000: '0'} != {2000: Decimal('0')}
- {2000: '0'}
+ {2000: Decimal('0')}

It is desirable to support high-precision inputs, optionally. forecaster must work with native (i.e. float-valued) numerical inputs. So rather than remove Decimal entirely from the test suites, we should revise the project to provide support and test appropriately.

There are three things to do to resolve this:

  1. Remove Decimal or Decimal-convertible inputs from the above tests (e.g. string inputs should be converted by client/test code, not passed to forecaster methods), so that native numerical types are used across the existing suite of tests.
  2. Add specific high-precision numeric library compatibility tests that provide Decimal input and test for output that is (a) correct numerically and (a) correctly typed (as Decimal). These can be high-level integration tests (i.e. one test, or a few tests, per module); there is no need to reproduce all existing tests with Decimal variants.
  3. Either revise forecaster classes to (a) allow for float-typed constants to be converted to high-precision numerical types by providing a conversion method (e.g. so client code can pass Decimal) or (b) expose arguments for internal constants to allow client code to override them with high-precision versions.

On reflection, 3(a) is probably the better choice, as 3(b) requires client code to have extensive knowledge of forecaster classes' internal mechanics.

ChrisCScott commented 2 years ago

On review, it appears that at least some high-precision libraries do not support operations with float operands (e.g. Decimal(5.0) + 5.0 raises an error), but do support operations with int operands - which makes sense, because int is an exact representation. So consider allowing internal code and arguments to be int-typed. (This will avoid numerous casts of 0 and 1 to high-precision types, which is cumbersome.)

ChrisCScott commented 2 years ago

Consider using contextvars for high-precision conversion. (See PEP 567 for more on this, as well as this Stack Overflow article.)

This might run afoul of the general prohibition on global variables, but it's how decimal manages consistent precision and it seems appropriate if we think of native-precision/high-precision operating modes as different states of the application as a whole. (Since it is necessary that if one part of the application is using a type like Decimal, then all parts of the application need to use it.)

This might need to be a separate issue. Since implementation of a fix for this issue is already most of the way to complete, it might be a refactor to be done after the initial implementation is working.

ChrisCScott commented 2 years ago

On further review, it looks like TaxCanada still needs to be updated. The easiest way to do this is likely to let a constants-loading class read in and convert constant values, as suggested in issue #80.

Re-opening until this issue is resolved.