Introduce ex-date field for dividend transactions to PP model#5439
Introduce ex-date field for dividend transactions to PP model#5439ZfT2 wants to merge 5 commits intoportfolio-performance:masterfrom
Conversation
- PDF Extractor with ex date example
buchen
left a comment
There was a problem hiding this comment.
Thanks @ZfT2 for taking the time. I added a couple comments.
About one thing I haven't made up my mind yet:
Do we call it dateEx or exDate?
I know why you named it dateEx - all dates start with the same prefix. But it just doesn't flow...
name.abuchen.portfolio/src/name/abuchen/portfolio/model/AccountTransaction.java
Outdated
Show resolved
Hide resolved
| { | ||
| super(label, item -> item instanceof TransactionItem tx // | ||
| && tx.getSubject() instanceof AccountTransaction atx && atx.getType() == type | ||
| && atx.getDateEx() == dateEx ? atx : null, properties); |
There was a problem hiding this comment.
Use equals method, the dates are not necessarily the same object.
Thinking about it. This is not the right approach. Let's have a Matcher on the transaction like for all other attributes.
There was a problem hiding this comment.
I've had some trouble with the attribute matcher for AccountTransaction, see comment below.
I removed it now.
...portfolio.ui/src/name/abuchen/portfolio/ui/dialogs/transactions/AccountTransactionModel.java
Show resolved
Hide resolved
...portfolio.ui/src/name/abuchen/portfolio/ui/dialogs/transactions/AccountTransactionModel.java
Outdated
Show resolved
Hide resolved
...ortfolio.ui/src/name/abuchen/portfolio/ui/dialogs/transactions/AccountTransactionDialog.java
Outdated
Show resolved
Hide resolved
name.abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/messages.properties
Outdated
Show resolved
Hide resolved
| optional google.protobuf.Timestamp otherUpdatedAt = 8; | ||
|
|
||
| google.protobuf.Timestamp date = 9; | ||
| optional google.protobuf.Timestamp dateEx = 18; |
There was a problem hiding this comment.
This is technically not wrong, but the ex-date is only a date (no time or timezone information). And the rest of the codebase is using int64 for this (for example the historical prices). Therefore I suggest to use that here as well.
The ex-date can never have a time. It is not like the instruments starts trading without at 10:42. And the AccountTransaction also carries the LocalDate (and not LocalDateTime).
There was a problem hiding this comment.
I know the ex date is only a date without the time portion, but I seems there is no protobuf type "date only" available...
OK, thanks for the hint with the historical prices, I did it now like they are handled.
name.abuchen.portfolio.tests/src/name/abuchen/portfolio/datatransfer/ExtractorMatchers.java
Outdated
Show resolved
Hide resolved
name.abuchen.portfolio/src/name/abuchen/portfolio/model/AccountTransaction.java
Show resolved
Hide resolved
|
Comment from #5420 (comment) @buchen wrote:
With more and more assets, traded, or to be traded, for 24hrs (e.g. https://www.nasdaq.com/solutions/market-data-apac/24x5 ), "only the date" hardly can be the right answer. There certainly is already, and soon to be precisely specified by exchanges, the exact ex-div instant (which doesn't have to be midnight, and certainly can be different for difference securities in different timezones). In general, there shouldn't be "dates" in the data model anywhere, as they're too unspecific and ambiguous. |
- fixes from PR feedback 1
- adjusted PDF Extractor with ex date example
I agree. Personally I like it with prefix, but it would not bother me to rename it... |
- fixes from PR feedback 2
|
@ZfT2 thanks for the change - looks good to me. I noticed the following: on Windows and macOS, the date picker does not allow to not select a date. Therefore, all dividend transactions now get an ex date - and usually today's date. on Linux, the date picker allows to choose no date - but here I prevent this at the moment (because for the other fields, I need a date). One could enable the Linux date picker also on the other operating systems, but personally I do not really like the dialog. To solve, this we could add a separate checkbox that enables/disables the exDate. Also, the exDate should not be after the payment date. When editing the payment date, the exDate should be updated if it is now after the update. Maybe we should address this in a separate pull request... But first let's think about @pfalcon's comment:
You have a point. We could store exDate as instant ( How do we apply this in calculations? In the Dividend Dialog:
When extracting from PDF documents:
In the file, we would store the UTC value or the exchange timezone?
What is the time we assume for a historical price?
Then: I wouldn't want to build a dialog where the user can enter ex-date with date, time, seconds, timezone where is not relevant today. In a (brief) search, I could not find any major securities exchange that records or distributes dividend ex-dates with hour/minute precision. Also it is not clear what would be the anchor for the corporate price adjustment. Maybe the strategy for now is to create the field as an instance (can hold old data in the future), but fill it with only the date (instead adding hours and minutes where we later do not know if they are entered consciously). In XML that is easy, for Protobuf less so. I have to think about this some more. |
Or we just clear the exDate when the payment Date is changed and then is behind the exDate? The separate checkbox to en/disable the exDate field sounds good, I can try to add this.
I am also not aware that there exits exDates with a specific time.
So maybe like in the commit before? Use the timestamp type for protobuf but in the application we just use the date portion, without time. |
|
@buchen :
Thanks.
My point was more about storage model. It doesn't have to immediately apply to calculations (specifically, to performance calculations). For as long as PP keeps day-level calculations, dividend calculations related to ex-date can stay such too (and if that's to change, apparently the first question to ask is how to calculate "intraday" capital gains performance, like something was bought, the sold 10 minutes later). However, storing exact ex-date timestamp will allow to unambiguously calculate to which trade a dividend payment belongs, which in turn will allow to calculate total return ("absolute performance") of a particular trade (i.e. period when number of shares remained constant). That's not what PP currently does, but is pretty useful metric IMHO. Random example: Comment #3450 (comment)
It's not realistic to expect that a portfolio tracker like PP would allow to record "accrued interest" (or "accrued dividend") at the time of security exchange. But there's no need, if the tracker can calculate total return. PP can, but on the level of security. However, a security purchased at different times shows very different performance characteristics, so having that "total return" metric on the level of individual trades is useful. And that again requires unambiguous way to attribute a particular dividend payment to a particular trade period.
Handling timezones is completely separate and different can of worms. I personally wouldn't argue for solving that anytime soon, and certainly not to lump it with just making sure [all] timestamps stored actually have "time" component. How it works currently is [apparently] by assuming that there's some "implied" timezone for each security (based on its exchange), and transaction timestamps, etc. match that timezone. I'd argue "that works".
Yes, that's certainly true. But then NASDAQ plans to go for 24/5 trading, and they will have to make it explicit when exactly ex-dividend event happens. And that change doesn't happens in "distant unspecified future", it happens this year, and not even end of it, but in "a few months". So yes, the proposal is to use LocalDateTime for the exDate field in the data/storage model, and use LocalDateTime arithmetic (comparisons, etc.) for operations on it. That will allow the data model to be future-proof and avoid unneeded migrations in the future. As for the immediate UI implementation, can certainly just show the date part of it. Pioneers who need intra-day precision, can start with editing XML directly before bringing up a case for UI elaboration. Thanks. |
My point was here: the binary format (protobuf timestamp) includes the timezone. I cannot decide not to handle timezone.
LocalDateTime is a runtime type question; it is not the persisted type question. I'd prefer to remember if the user (or the import) provided a date or provided date and time. Otherwise, the app has to start guessing later if the time was entered by default or assumed or if the timezone was picked or assigned by default. When using XML as a storage, the string either includes the date or date and time. In Protobuf, we could use a oneof to express one of date or one of timestamp. But the oneof expression can also be added backward compatible later. Therefore I would argue storing the date only is forward compatible (and fits to that exchanges only publish dates, the user can enter only a date, the import only creates dates).
I agree that this is an issue. Particularly problematic if one instrument is held in different securities account, but dividends are recorded on the same money account. And in trades, dividend payments are not included at all at the moment. However, I do not think the ex-date alone will solve this. It is primarily an assignment issue. What the ex-date could solve is the performance of dividends that are paid after the instrument was already sold. There I currently have the problem of a non-existing position generating a return (which is then a division by zero). With an ex-date (and correctly recorded sales transaction), I could record the performance on the ex-date instead. |
|
@buchen :
I keep forgetting about Protobuf complication. But then I'd argue it's only more important to choose the (most) generic format right away and avoid need for any migrations, even if they're "automagic" (only Protobuf specialist would know all those details). Anyway, if Protobuf doesn't support "not-TZ aware datetime type" (that's what I meant by "storing as LocalDateTime"), that's sad. But the obvious workaround is to use e.g. UTC, for as long as that fake TZ still can be imported as non-TZ aware LocalDateTime.
Oh, that sounds too complicated for sure. If the choice is between supporting only date, or supporting datetime, but additionally tracking whether it's only date or actual datetime, then "only date" is the simple route. However, what I argue, is that "store as datetime" is a better choice, even if current PP's builtin UI treat it as date-only. That argument is based on my experience with extending LatestSecurityPrice to have higher precision. So, if latest price is from today, that doesn't say much. Is it from a minute ago? Then it's fresh and probably doesn't even need an update? 5 minutes ago? Maybe worth update. A few hours? Certainly needs an update. Beginning of the session? Likely pretty irrelevant by now. Maybe the price is from pre-market session at all? Well, it was useful during pre-market, but certainly useful when real session started. Finally, is price from market session, but it's post-market now and earnings release happened? Then the price is useless again, and there certainly repricing after the earnings. So, LatestSecurityPrice is prime example where date-only limits usefulness considerably, and prime example of what needs to be converted. However, doing the migration isn't so easy. The biggest issue is that LatestSecurityPrice inherits from SecurityPrice, and its date field comes from it. Obviously, I won't argue that SecurityPrice should get second-level timestamp, given that it's biggest storage and memory hog. What I did locally is to add separate datetime field, then add method overrides which try to maintain those fields in sync. But maybe I didn't do it thorough enough or something, but in XML, there're still regular discrepancies, so I find that in my scripts, I always need to munge both fields, trying to find which has more relevant information. In this case, the "right" (but not backwards compatible) solution would be to break inheritance from SecurityPrice and just switch to use datetime. And for all future cases, never use unspecific date-only fields, but always use datetime fields, even if a usecase for the "time" isn't immediately here - it certainly comes somewhere in the future.
Yes, that's what I meant, and I think having proper ex-datetime would allow to do that even on the level of individual trades (future-proof). It's hard to say for sure if it will, some experimenting and prototyping would be required. However, if ex info would be stored as date only, someone could think "oh, even if that worked out now, chances that it would break later when 24/5 trading is enabled, so it doesn't even makes sense to start experimenting if specific underlying infrastructure is not there". |
Okay, now I better understand where you are coming from. (And PP has no API to "extend" but of course one can "fork" the code). Adding just the time (00:00 by default) should not hurt. When editing without the time (the initial implementation), the ex-date refers to the first day trading without. So midnight is okay. One can also build a custom type in Protobuf. |
I would like to highlight a current limitation in PP regarding bond transactions. Currently, there is no dedicated field to input the Accrued Interest (or "accrued dividend") paid to the seller at the time of purchase.
This results in an incorrect Capital Gain/Loss calculation when the bond is sold or redeemed, as the accrued interest should be treated as an income adjustment, not a capital cost. Why not Since PP currently records coupon payments as "Dividends," there should be an option to enter a "Negative Dividend" (or a specific "Accrued Interest Paid" entry) at the date of purchase? This would allow PP to:
Thanks |
Issue: #5439 Signed-off-by: XY <xy> [use custom Protobuf type; split commits; rebased to master] Signed-off-by: Andreas Buchen <andreas.buchen@gmail.com>
… ex-date Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…idend ex-date Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… ex-date Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… ex-date Issue: portfolio-performance#5439 Issue: portfolio-performance#5502 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… ex-date Issue: portfolio-performance#5439 Issue: portfolio-performance#5502 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Issue: portfolio-performance#5439 Allow editing the ex-date in the account transaction dialog portfolio-performance#5511 - initial check-in
Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…idend ex-date Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…idend ex-date Issue: #5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dend ex-date Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Issue: #5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dend ex-date Issue: #5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… ex-date Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Issue: #5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… ex-date Issue: #5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…date Issue: portfolio-performance#5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Issue: #5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…date Issue: #5439 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
ex-date is available in the model. |
continues #5420
Here is the very first version , I have added the new field ex date to the model.
What should work:
open: