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

incorrect prices for "daily" roi #2199

Open
aragaer opened this issue May 22, 2024 · 8 comments
Open

incorrect prices for "daily" roi #2199

aragaer opened this issue May 22, 2024 · 8 comments
Labels

Comments

@aragaer
Copy link
Contributor

aragaer commented May 22, 2024

Slightly modified snake oil example:

P 2019-09-30 SNKOIL $1.075

2019-12-20 Investing in Snake Oil
 assets:cash
 investment:snake oil   100 SNKOIL @@ $107.5

P 2019-12-23  SNKOIL $1.08

2019-12-23 Fear of missing out
 assets:cash 
 investment:snake oil   83.33 SNKOIL @@ $90 ; $90/$1.08 = 83.33

;P 2019-12-23  SNKOIL $1.09  ; -- this line affect the 2019-12-22 ROI result

; Recording the growth of Snake Oil
P 2019-12-24  SNKOIL $1.1

2019-12-25 check
 investment:snake oil   0 = 183.33 SNKOIL

The issue here is that there is another (commented in the example) price directive for 2019-12-23. When doing a per-day roi I'm expecting that the final price for a date would be the first price of the next day, however the last price is used instead. Adding a price directive to reflect that the price changed after the additional purchase affects the calculated roi of previous day.

Apparently it is not directly related to roi, just the general case of using the next day as "end" of a period and using the last price when multiple directives are given for the same date.

@simonmichael
Copy link
Owner

Thanks @aragaer, I imagine @adept will have thoughts on this at some point.

@adept
Copy link
Collaborator

adept commented May 24, 2024

Roi does not interact with pricing directives directly, it just asks pricing functions to price things. So it would be very hard to change how Roi behaves here without changing the behaviour of the rest of the hledger.

I think this is the case of "multiple pricing directives per day are generally not (very well?) supported in hledger"

@simonmichael
Copy link
Owner

Ah, yes. I guess that is probably what you're seeing. Valuation (pricing) in hledger uses a single P directive per day, the last one parsed if there is more than one, I am guessing. I found slight mention of it at https://hledger.org/dev/hledger.html#finding-market-price .

@simonmichael
Copy link
Owner

@aragaer did that explain it ?

@aragaer
Copy link
Contributor Author

aragaer commented Jun 9, 2024

Yes, this is exactly how I understood it. The question is -- should it be fixed? Is it worth implementing a different valuation request which will use first price instead, so that it is used for "end" valuations?

@simonmichael
Copy link
Owner

simonmichael commented Jun 10, 2024

I think when there are multiple similar directives, generally the last one wins. Similarly to when there are multiple similar command line options, the last one wins. So it sounds like the current behaviour is at least consistent with hledger's usual way of doing things ?

@aragaer
Copy link
Contributor Author

aragaer commented Jun 10, 2024

When calculating roi for certain period we want the price at the end of period. Current implementation is to use the price for the first day after that period, which means that first directive would be more appropriate. Unfortunately there is currently no way to specify "end price".

@adept
Copy link
Collaborator

adept commented Jun 12, 2024

I dont this that this lines up with how the pricing code works, btw.

But regardless, I think that support of more than one price per day would require a lot of forward planning, cause opportunities for tricky edge-cases would expand significantly.

Nevermind first and last price of the day, what about having 15 price directives for a given day, interspersed with transactions? What if those prices (and transactions) come from different files via include directives, and include graph has "diamonds"? Etc etc

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

No branches or pull requests

3 participants