Income class tests need to be fixed or our math is the wrong precision

[nonprofittechy]

====================================================================== FAIL: test_asset_list (docassemble.ALToolbox.test_al_income.test_correct_outputs)

Traceback (most recent call last): File "/home/quinten/docassemble-ALToolbox/docassemble/ALToolbox/test_al_income.py", line 109, in test_asset_list self.assertEqual(Decimal("1234567.89"), asset_list.market_value(source="home")) AssertionError: Decimal('1234567.89') != Decimal('1234567.889999999897554516792')

====================================================================== FAIL: test_income (docassemble.ALToolbox.test_al_income.test_correct_outputs)

Traceback (most recent call last): File "/home/quinten/docassemble-ALToolbox/docassemble/ALToolbox/test_al_income.py", line 49, in test_income self.assertEqual(Decimal("121.2"), income.total()) AssertionError: Decimal('121.2') != Decimal('121.1999999999999957367435854')

====================================================================== FAIL: test_income_list (docassemble.ALToolbox.test_al_income.test_correct_outputs)

Traceback (most recent call last): File "/home/quinten/docassemble-ALToolbox/docassemble/ALToolbox/test_al_income.py", line 74, in test_income_list self.assertEqual(Decimal("511.16"), income_list.total(1)) AssertionError: Decimal('511.16') != Decimal('511.1600000000000214583906200')

This implies that we're storing numbers with the wrong precision somewhere, or we're not converting to Decimal at the right place in the calculation.

[nonprofittechy]

Maybe this function is the problem:

def_currency_float_to_decimal(value: Union[str, float]) -> Decimal:
"""Given a float (that was set by a docassemble currency datatype, so
    rounded to the nearest `fractional_digit` decimal places), returns the
    exact decimal value, without floating point representation errors
    """
ifisinstance(value, float):
# Print out the value of the float, rounded to the smallest allowable amount in the
# locale currency, and use this value to make the exact Decimal value
digits= get_locale("frac_digits")
returnDecimal(f"{​​​​​​​​value:.{​​​​​​​​digits}​​​​​​​​f}​​​​​​​​")
else:
returnDecimal(value)

it should possibly be changed to a fixed precision instead of "smallest available locale precision"

Also these lines don't seem correct:

        self.assertEqual("6.30", str(value_float.total()))

        self.assertEqual("15632.76", str(job.gross_total()))

if we're allowing arbitrary number of decimals, the string values will not be equal. they can have trailing zeros

[BryceStevenWilley]

Note that this is failing because you don't have LANG set. See https://github.com/SuffolkLITLab/docassemble-ALToolbox/blob/fd3bfe53748828bcb4f78ffa3de1abec1454d5d1/.github/workflows/unit-tests.yml#L8. DA has this set in all of it's places: https://github.com/search?q=repo%3Ajhpyle%2Fdocassemble+export+LANG%3D&type=code.

The second comment is more information on https://github.com/SuffolkLITLab/docassemble-ALToolbox/issues/104. It's useful for adding other locale support, but it's not a priority for me ATM.

It does look like the root of your issue is that the default locale has 127 digits. We could add special code to avoid that issue and default to 2 (which was the number of currency digits for most locales IIRC), but I'm hesitant to add hacky code like that.

[nonprofittechy]

I'm just trying to understand why this code depends on locale. Is it always safe to assume the server locale is the one we want to use for decimal calculations?

IMHO, this should be 2 decimals, which is what the tests assume. If we want to expand to support other decimal lengths, we need to adjust the tests. But I think locale support is out of scope for this module.

If we really want to keep different decimal lengths as an option, my opinion is that it should be optional behavior, not a side effect of changing the server locale that can't be overridden. Perhaps we can add a configuration option. [Edit: maybe even better to make decimal precision a class parameter]

Given prioritization, I'd suggest we just remove that flexibility for now.

[BryceStevenWilley]

Is it always safe to assume the server locale is the one we want to use for decimal calculations?

Yes, because it's the only currency that we can ask for / show in questions. That's one of the reasons that I made this code depend on locale, because the docassemble functionality depends on locale. From the docassemble docs:

In addition, the input box shows a currency symbol based the locale defined in the configuration.

I agree that interviews that ask for currency from other locales would be nice, but it can't be done with al_income at the moment. You'd have to make your own currency widget, and thus all of the YAML questions we have.

IMHO, this should be 2 decimals, which is what the tests assume.

Again, this is because the tests aren't done. https://github.com/SuffolkLITLab/docassemble-ALToolbox/issues/104 is the issue I made to track testing other locales.

Perhaps we can add a configuration option. [Edit: maybe even better to make decimal precision a class parameter]

I'm strongly against adding extra complexity to these classes. Fields with datatype currency already have to be the locale's currency. Making a separate param somewhere that needs to be the same as that locale or it'll be wrong doesn't help anyone IMO.

Given prioritization, I'd suggest we just remove that flexibility for now.

I'm not sure why it needs to be removed. I know the tests aren't complete, but internationalization with Assembly Line packages is already unsupported / experimental.

[nonprofittechy]

I didn't realize Docassemble doesn't currently let you show the currency input in a different locale from the server locale. But I don't know if we want to tightly couple to that behavior.

The decimal precision an author would ideally use for a form is going to depend on the form's expectations, not the language that is shown to the user. It should be up to the form author, not the server that the form is installed on.

How important is it to mirror what Docassemble's currency datatype does? In theory this class can be used without it. How much engineering effort is it worth to put in to handling changing locales and the extra testing to handle it correctly?

I hear what you're saying about the risk of having too many configuration choices. But it really seems like following the server setting without any flexibility is worse. If we want to follow the server's choices, it should happen in the interview YAML, not in the module.

[BryceStevenWilley]

I agree on most of that, it's just a lot of effort to convince upstream to change these things (if you can at all), or to write our own currency widget that takes custom currencies. If you're implying that hard-coding things back to 2 decimal places as you suggested earlier is better that tying that number to the server settings, I don't agree with that. My view is that docassemble is flawed, but on single locale servers, it's still workable. I see it as a 1 server to a jurisdiction, and jurisdictions are typically not multi-locale (at least the ones we're targeting in the US, and maybe some single countries in Europe, definitely not EU wide jurisdictions though).

The decimal precision an author would ideally use for a form is going to depend on the form's expectations, not the language that is shown to the user.

The language shown to the user is not the same as the locale. Locale is still tied to the server, but it "primarily controls the default formatting of currency and numeric values", not the language. It is confusing, as most of the times in the OS these are the same language+region setting, but they seem to be distinct in DA.

You're welcome to change what you want. I just made https://github.com/SuffolkLITLab/docassemble-ALToolbox/pull/225 so the tests are a bit clearer and show what locale they expect.

[nonprofittechy]

I'm not totally convinced, but I am convinced that you already implemented the workaround in #225 so we'll go with the path of least resistance for now :)