redstreet / fava_investor

Comprehensive set of reports, analyses, and tools for investments, for Beancount and Fava (plain text, double entry accounting software). Includes Fava plugins, command line tools, and libraries for each module.
GNU General Public License v3.0
135 stars 19 forks source link

Asset Allocation by Class does not convert prices transitively #82

Closed korrat closed 1 year ago

korrat commented 1 year ago

With the following minimal example:

plugin "beancount.plugins.auto"

option "operating_currency" "EUR"

1980-01-01 custom "fava-extension" "fava_investor" "{
  'asset_alloc_by_class' : {
      'accounts_patterns': ['Assets:Investment'],
  },
}"

2023-01-01 price USD 2 EUR

2023-01-01 *
    Assets:Investment:SOME-TICKER              10.000 STCK {1 USD}
    Assets:Checking                           - 5.00  EUR @ 2 USD

When running fava-investor, I get an error:

$ investor assetalloc-class .\main.bean
Error: unable to convert 10.000 STCK to base currency EUR (Missing price directive?)

Running the following beancount query works:

$ bean-query.exe .\main.bean 'SELECT account, convert(sum(position), "EUR")'
           account            convert_s
----------------------------- ---------
Assets:Investment:SOME-TICKER  5.00 EUR
Assets:Checking               -5.00 EUR

This suggests, to me, that fava-investor cannot make use of transitive pricing relations. Could this be added?

redstreet commented 1 year ago

It does work already. See #32 on how to configure it.

korrat commented 1 year ago

Thank you for pointing me in the right direction. I should have checked the examples more carefully.

I took a look at the asset allocation module and came across this line: https://github.com/redstreet/fava_investor/blob/661feddd31b7f37d2dd7c783acb49c2506eaab2a/fava_investor/modules/assetalloc_class/libassetalloc.py#L200 Would it be possible/sensible to reduce the inventory directly using convert.convert_position? This way, we would get conversions via the cost currency “for free” when there is no direct conversion possible.

This would be equivalent to the BQL query I gave.

redstreet commented 1 year ago

Np at all, the doc on this is not obvious. I just added a reference to #32 to the README.

Reg convert.convert_position: interesting, and good to know. It's been a while since I wrote this, and I can't recall if that's something I tried and know doesn't work, or if it's something I never considered.

Have you tried it? If you can confirm that it works as expected, would you be up for submitting a PR, possibly with new test cases if needed? Thanks!