-
Notifications
You must be signed in to change notification settings - Fork 623
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
Improve ATH dashboard widget to support all dataseries #3486
base: master
Are you sure you want to change the base?
Improve ATH dashboard widget to support all dataseries #3486
Conversation
…d not only securities
…dapted translations
Totally forgot that there is already another PR from @RomanLangrehr with a similar change 🤦 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request.
In general, I like the idea to extend the ATH widget.
But I do not think using the PerformanceIndex instead of security prices works:
- the performance index fills in values to complete the data series
- benchmark series are calculated relative to the underlying series
- using the performance index also changes the semantics slightly - it includes dividends (if it is not a benchmark)
PerformanceIndex index = data.calculate(ds, period); | ||
AllTimeHigh ath = new AllTimeHigh(index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use the PerformanceIndex
here, we will get data "filled" in for the given interval. You can see this in the diagrams. If there are no current prices, then the lines in the diagram are flat. Therefore the "last" data is misleading - it is always the last date of the interval, not necessarily the last date of the underlying data series. For example, my test file is missing the data for August, but still the tooltip shows 9 August as the current data point.
The previous code was working against the security prices and did not have this issue
}) // | ||
.withBenchmarkDataSeries(false) // | ||
.with(ds -> ds.getInstance() instanceof Security) // | ||
.withBenchmarkDataSeries(true) // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the intention to add benchmark series. However, the benchmark is always calculated against the underlying main series (here: Gesamtportfolio). That is not a problem, if both data series have data for a given interval. But it is a problem if the data for an interval does not match. Then the data is transformed.
(Background: if I compare my portfolio against the DAX, then both data series should have the same starting point in the performance diagram - regardless of the fact that the DAX exists for much longer)
/ Values.Quote.divider(), | ||
Values.Date.format(security.getSecurityPrice(LocalDate.now()) | ||
.getDate())); | ||
formatter.apply(ath.getValue()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you check for null values above, then I think we need to check for null values here as well
I read @RomanLangrehr change in #3037 correct, then his pr does ATH on the performance (accumulated percentage). Thinking about the requirements: One issue I see with the current code (=before your pull request) is the data series selection: The algorithm could easily work with any security (because it is using the security prices), but piggy-backs on the data series selection for instruments (and because one cannot purchase an index such as the DAX, it currently does not show up). Maybe this is the first thing to fix. The current change I find confusing from a user point of view:
Then: Let's say we additionally add composite data series (Gesamtportfolio, taxonomy-based data series) but keep the security ATH as is (=against the security price). This difference is also not trivial to understand: let's say I have a portfolio with only one instrument. Then the ATH for this portfolio is not identical to the ATH for the instrument itself - the former includes dividends, the latter does not. I would love to read your reaction to my thoughts. I think more about it, but I could see the following option:
|
@buchen thank you for your detailed comments on this PR, very much appreciated and again what learned. You're right, probably I was too quick switching the implementation to the
Ah yes, right 👍 Personally I find it more interesting to see the distance from the total, e.g. for Gesamtportolio.
Hm perhaps I am having problems with the PP domain language here: so with "instruments" you mean "invested securities" whereas indices are "non-investable securities"?
Good question. From my perspective it makes no sense to calculate e.g. the Gesamtportolio ATH excluding dividends. But yes, this must then be made more clear in the description somewhere.
So basically the contents of the "choose benchmark series" dialog? Not sure if there is really a need to have indices as well there. Of course we could implement that, but I haven't noticed this as a request in the forum or by myself.
This is the more important feature I think. From my usability perspective I would love to only have 1 ATH widget which does cover both cases (because the use case is "distance to ATH" no matter of the underlying data series), but I also understand that it might be too confusing or requiring too much change in the data series selection. What do you think about modifying this PR to:
But of course I'm also fine with implementing a separate widget as you suggested if you like to. |
Hi, thanks for PortfolioPerformance, great and stable app. I see that there are conflicts now, but since being able to see the ATH delta for the whole portfolio would be an interesting metric and currently it just displays 0%, I would greatly appreciate if this could be considered for inclusion in one of the future releases. |
This small PR improves the All-Time-High (ATH) dashboard widget to support all kind of data series (e.g. whole portfolio, benchmarks, securities).
In addition it slightly adapts translations and moves the dashboard menu entry into subcategory "Performance" to better reflect its purpose.
https://forum.portfolio-performance.info/t/all-time-high-ath-des-ganzen-portfolios/24712