Preserve costs during tax adjustment

[korrat]

Previously, tax adjustment discarded the cost of positions, causing beancount.core.convert.convert_position to miss some conversion paths via the cost currency. This necessitated a fallback to operating currencies for finding viable transitive conversions.

With this patch, tax adjustment preserves the cost of positions. Therefore, the cost currency is still available for automatic detection of transitive conversions when computing the asset allocation.

One important assumption is that tax adjustment only ever encounters costs after realization, as having a cost spec for the total cost would change the cost per unit. This assumption is checked via assert and has been manually tested without error on the multicurrency example.

The patch leaves the fallback logic for conversion via operating currencies in place as an alternative when conversion via cost currency fails.

Fixes #82

[korrat]

Unfortunately, I was unable to get the test suite running on my system, so I was unable to test automatically. I compared the output on both the huge and multicurrency examples, which was unchanged before and after the change.

If desired, I can also amend this patch or add another patch to show both options, conversion via cost currency and operating currency, in the multicurrency example.

[redstreet]

Nice patch, and looks good to me! I tested it with a couple of rudimentary cases, and and it works fine. Couple things:

1. Shouldn't this patch let us remove via= here? If so, would you mind removing it?

2. There's a couple minor linting issues

BTW, I simply run pytest to run the unit tests. If this doesn't work, please let me know?

Thanks again!

[korrat]

Yes, we could remove the line you linked. However, it could still be a useful fallback behaviour for some edge cases. For example, consider a case where neither a direct conversion between the position and the base currency nor between the cost currency and the base currency exist, but an indirect path via another operating currency exists. For instance, this small modification of the multicurrency example, highlights the behaviour:

option "operating_currency" "USD"
option "operating_currency" "GBP"

2010-01-01 open Assets:Investments:Taxable:XTrade
2010-01-01 open Assets:Bank

2010-01-01 commodity SPFIVE
 asset_allocation_equity_domestic: 100

2010-01-01 commodity SPUK
 asset_allocation_equity_international: 100

2011-01-10 * "Buy stock"
 Assets:Investments:Taxable:XTrade 100 SPFIVE {5 USD}
 Assets:Bank

2011-01-09 price GBP 1.5 USD
2011-01-10 * "Buy stock"
 Assets:Investments:Taxable:XTrade 100 SPUK {5 EUR}
 Assets:Bank

2011-03-02 price SPFIVE 5 USD
2011-03-02 price SPUK   5 GBP
2011-03-02 price GBP 1.5 USD
2011-03-02 price EUR 0.7 GBP

I'm not sure how useful it would be in practice, but leaving the fallback in place would ensure backwards compatibility. That said, I can add a patch removing the fallback if you'd prefer.

[korrat]

When I run pytest, I get PermissionErrors, saying that the tests cannot access some temporary files. After a bit of reading, I think the issue comes down to me being on Windows. On Windows, the file returned by tempfile.NamedTemporaryFile (which is used to implement beancount.utils.test_utils.docfile) cannot be opened a second time.

[korrat]

I can squash the commits before you merge if you prefer. Just let me know.

[redstreet]

Makes sense, thanks for the explanation and the case where the fallback is needed.

Nice find and patch, great documentation, thanks again, merged!