Securities Account chart with Invested Capital, Delta #3969
Conversation
That is indeed super strange. I was able to debug it to these lines below: If the pane has buttons (and the chart pane has buttons as opposed to the account balance pane), then somehow the width of the chart element passed on to the adjust range method is way too small. And then adding a couple percentages margin makes the chart show up in the center. I fixed that by reversing the order - first showing the chart, then updating the toolbar. Not intuitive. Apart from this, I'll like your proposal. However, right now the calculation of the data series happens in the UI thread. For the account balance, this is not so dramatic because that it is fairly cheap. For the portfolio balance, this can become a problem because that can be an expensive calculation. Let me change that (possibly for both) |
|
Ah great, well done for the bug fix ! (-> no side effect on SecurityChart as If this is non UI calculation is also updated for AccountsBalance chart, then this means we could enhance it too with Interest metric for example ! One point to confirm maybe : the currency of the charts. I used |
When the toolbar buttons are updated first, the adjust range method in the chart within the pane content initially receives an incorrect pixel# width. Issue: #3969
* Compute the data series in a background thread * Used shared PerformanceIndex to setup data series Issue: #3969
| private double[] toDouble(long[] input, double divider) | ||
| { | ||
| double[] answer = new double[input.length]; | ||
| for (int ii = 0; ii < answer.length; ii++) | ||
| answer[ii] = input[ii] / divider; | ||
| return answer; | ||
| } | ||
|
|
||
| private double[] accumulateAndToDouble(long[] input, double divider) | ||
| { | ||
| double[] answer = new double[input.length]; | ||
| long current = 0; | ||
| for (int ii = 0; ii < answer.length; ii++) | ||
| { | ||
| current += input[ii]; | ||
| answer[ii] = current / divider; | ||
| } | ||
| return answer; | ||
| } |
There was a problem hiding this comment.
I believe it makes sense to make these methods reusable. There are multiple places where the same code exists.
| PerformanceIndex index = PerformanceIndex.forPortfolio(client, converter, portfolio, | ||
| Interval.of(dates[0], dates[dates.length - 1]), warnings); |
There was a problem hiding this comment.
The PerformanceIndex already contains all data series. I changed it so that we calculate the index once, and - if configured - extract a data series from it.
|
Hey @mierin12, I have made a couple changes - not it is rebased and merged.
Nice change. |
I agree to using the base currency. I think all calculations should be in the "reporting currency" (= base currency). If the user wants to see it differently, it is easy to change via the menu or from the statement of assets view. I think the currency of the reference account does matter here. Personally, I am thinking about getting rid of this reference account. It is used to collect dividend statements for the instruments in the brokerage account. But there could be better ways to do this (for example, right now it is possible to change the reference account, and then the link of the old payments is lost. Or what if payments are done in multiple currencies? Then only some are picked up). Therefore I try to avoid adding even more dependency on the reference account. For cash accounts it is different. There you want to see the balance in the currency of the account. |
Intersting point. I do not really know. Maybe because initially no security is selected (but an account is selected). I do not want to spend the time debugging now - with the updated code, the security price chart pane continues to work as expected. |
|
Hello @buchen , many thanks, I hope the feature will be liked. I will have a look on your update to better understand the 'calculated in the background' and PerformanceIndex advices to be able to do better next time. One comment, are the transactions not sorted anymore ? I remember I add to specifically put the sort before calculating the start date from the tx list, because I had a case with a Account with a Transfert of security which somehow was taking the first place of the transaction list while not being the oldest transaction of the portfolio. So start date was at first this Transfert date and not the correct start date. In AccountBalanceChart the sort is after start date but the problem seems to arise only for Transfert of security so it was not impactful. I do not have my computer with me at the moment to test.. |
as portfolio.getTransactions is not sorted by default Issue : portfolio-performance#3969
as portfolio.getTransactions is not sorted by default Issue : #3969
Hello,
Proposition:
In the same way than Deposit Accounts have a pane with an Account Balance Chart, this PR is proposing to add a Securities Account Balance Chart (first commit).
Actually, the real goal is also to associate to those Portfolio Charts the Invested Capital and Delta metrics (+accumulated Taxes and Fees and more if relevant) that a lot of users are looking for (second commit).
It is limited in scope compared to #3754 which should be the general correct solution for those common metrics, but I think here the implementation is avoiding the "multiselection" and UI questions of #3754.
The metrics are here associated to a Portfolio only (no association to Taxonomy, Custom Filter Portfolio+Cash account etc) but I think this is nevertheless already providing some useful data.
Implementation : similar to the
AccountBalancePane,AccountBalanceChartand to theSecuritiesChart.Caveat: by using a similar structure than
SecuritiesChart, the plotted data series are not customisable (color, area vs line, linestyle etc).I mostly used the color defined in
DataSeriesSetbut "less gray" colors could be used.Results :
Example from kommer file:
custom account
Bug : this PR is in Draft as there is at least one bug that I did not manage to fix. When the graph pane is selected from the Statement of Assets or Transactions panes of a Securities Account, the chart is somehow too big for the plotted data :
It updates correctly when changing the metric in the chart menu or when switching between Securities Account.
If someones has an idea on this bug or a feedback, it is much welcome.
(Other ideas in case of next steps: in the same way, Earnings/Interests could be added to the
AccountBalanceChart, Delta could be a bicolor positive/negative area as is in theSecuritiesChart, "account events" may be added in those charts.)