Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't exclude unrealized transactions when calculating net worth #467

Closed
wants to merge 1 commit into from

Conversation

xentac
Copy link
Contributor

@xentac xentac commented Jan 10, 2017

Fixes #453. Seems that filtering unrealized transactions when calculating holdings makes sense, but not when calculating the Balance Sheet's chart's net worth values.

@yagebu
Copy link
Member

yagebu commented Jan 10, 2017

I'm not using the unrealized gains plugin, so I might be wrong here, but as far as I can tell, it will insert only one (set of) transaction(s) after the last transactions. How could this affect the net worth chart (except for maybe the last data point)?

Also, currently (in Fava and Beancount) the net worth for a given operating currency is the sum (over all holdings) of all market values converted to the operating currency. So it is consistent to use the same list of entries (i.e. all but the unrealized ones) - why should we differ from Beancount here?

@xentac
Copy link
Contributor Author

xentac commented Jan 10, 2017

My unrealized plugin is an improvement on the default one (https://github.com/xentac/beancount-plugins-xentac/blob/master/beancount_plugins_xentac/plugins/unrealized_periodic.py). It posts unrealized gains every month. That way I can see how my net worth is affected as time goes on.

Can you point me to the code that uses market value and not cost? I thought that all commodities were being rendered at cost and not at market value, which is why the unrealized plugin was necessary to begin with. The whole reason I started using the unrealized plugin was because I was seeing all my commodities at cost and not seeing their market values.

@xentac
Copy link
Contributor Author

xentac commented Jan 10, 2017

(Original discussion where I got this impression #129)

@yagebu
Copy link
Member

yagebu commented Jan 10, 2017

It posts unrealized gains every month. That way I can see how my net worth is affected as time goes on.

Monthly unrealized gains shouldn't be necessary for the net worth chart as it should already take into account the market value.

Can you point me to the code that uses market value and not cost?

The convert_inventory function in fava.core.holdings. It first determines the price (in the cost currency, if there is one), then the price of the cost currency in the target operating currency and calculates "number * price * price_of_cost_currency" (the first two factors are the same as the market value).

I thought that all commodities were being rendered at cost and not at market value, which is why the unrealized plugin was necessary to begin with.

That's true in e.g. the Balance Sheet and Income Statement but not for the net worth chart, which is a holdings-like report (and calculated as explained in my last post). Sorry that I didn't express this clearly in #129 - there I was only referring to the account tree tables.

@xentac
Copy link
Contributor Author

xentac commented Jan 10, 2017

Ok then. I see. I'm a little sad that my net worth is not as high as I thought it was ;)

In that case, I will probably drop my usage of the unrealized plugin. I am a bit sad that I don't currently have a way to look at an individual account's market value. But this does fit decently well with my original plan of having a market value vs. cost value toggle on the balance sheet to make it easier to see both sets of values.

If I recalibrate my thinking based on this data, it looks like the balance sheet and account monthly changes/balances display cost value. What do you think about adding a toggle somewhere that displays market vs cost values everywhere? It would be less confusing than this half one way and half the other way scenario.

I didn't realize there was already code in fava to do market conversion, a patch like that should be much easier then.

@yagebu
Copy link
Member

yagebu commented Jan 10, 2017

A toggle like you describe would definitely be useful. I'm just not sure where it should go and if it being a "global" thing would be the best idea. 'Balance Sheet' has a specific meaning and if we display the market value it would rather turn into a "holdings account tree" (which would be useful, but maybe belongs on a different report)

I didn't realize there was already code in fava to do market conversion, a patch like that should be much easier then.

I'm pretty sure there's a function in Beancount that calculates the market value (for the net worth, we might need an additional conversion, so it's done in Fava)

@xentac
Copy link
Contributor Author

xentac commented Jan 10, 2017

See the discussion here: https://groups.google.com/d/msg/beancount/zK6sFnzY4fg/msTTPVDTCwAJ

It does have various ways to do it but nothing canonical and nothing to evaluate at a specific date. Martin has started to refactor it, but he's not done yet.

I think having separate reports might be a bit tricky because the "holdings account tree" would have to link to a "holdings account journal" but the balance sheet might link to the regular account journal. That's why I was thinking that an always visible toggle would make the displays less ambiguous.

I'll leave it to Martin to argue whether we should hew strongly to accounting terms with beancount.

@yagebu
Copy link
Member

yagebu commented Jan 10, 2017

There's already a get_inventory_market_value function...

@xentac
Copy link
Contributor Author

xentac commented Jan 11, 2017

I'll start a patch that creates a different holdings accounts (maybe call it Market Balances or something) page and adds two new Changes/Balances links in the account journal so we can see what it looks like and kick the can down the road on the global toggle decision.

@blais
Copy link
Member

blais commented Jan 14, 2017

prices.get_inventory_market_value() and prices.get_position_market_value() will disappear. They're being refactored into something nicer. Tune in to branch "conversions". I'm removing the various incomplete conversion functions from all over and centralizing them all into beancount.core.convert. (Ongoing.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants