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

Add derived data series in Statement of Assets Chart #4235

Conversation

mierin12
Copy link
Contributor

@mierin12 mierin12 commented Sep 16, 2024

Hello, this is a proposition to add derived data series to the statement of assets chart.

Closes #605
Closes #707
Closes #1529
Issue : https://forum.portfolio-performance.info/t/delta-diagramm-fur-ausgewahlte-aktien/6787

Results :
Capture d’écran 2024-09-15 202846

This is based on the PR #3754 from @shivampaw. As it was without update since February, I am proposing some modifications taking into account the previous review.

Main updates from #3754 :

  • Dedicated selection dialog for the derived data series.
    2024-09-15 20_25_35-

  • In the new data series selection dialog, I am proposing a different tree order :

    • [derived data]/Accounts & Portfolio/Account
      Capture d’écran 2024-09-15 202634
      I find it more intuitive than
    • Accounts & Portfolio/Account/[derived data]
  • Securities are added as a possible subtype of the derived data. I guess this is well applicable/defined ?

  • The derived data series are correctly plotted as bar or with an area when this is relevant. This was missing.

Note on taxonomy labels :
For the selection labels, two propositions (without/with commit 8120e87) :

  • with fullPath that I prefer but which is different than the existing behavior,
    Capture d’écran 2024-09-15 205300
  • with the same behavior as the existing data series selection
    Capture d’écran 2024-09-15 204955

Multiselection topic :
There is no multiselection issue I believe.
Multiselection allows here to plot several distinct derived data series.

Legend label :
Lots of ( ) maybe another format could be used ?
Delta (for the reporting period) (Portfolio A)
Delta (for the reporting period) [Portfolio A]
?

Other forum threads

Issue: https://forum.portfolio-performance.info/t/customize-filter-invested-capital/15474
Issue: https://forum.portfolio-performance.info/t/delta-fur-klassifizierung-anzeigen/15669
Issue: https://forum.portfolio-performance.info/t/graphs-of-investment-and-performance/23365
Issue: https://forum.portfolio-performance.info/t/filtermoglichkeit-allgemeiner-datenreihen/25460
Issue: https://forum.portfolio-performance.info/t/add-invested-capital-in-one-account-to-charts/10854
Issue: https://forum.portfolio-performance.info/t/investiertes-kapital-eines-depots-anzeigen-lassen/20766
Issue: https://forum.portfolio-performance.info/t/delta-im-berechnungszeitraum-fur-einzelne-depots/17238
Issue: https://forum.portfolio-performance.info/t/investiertes-kapital-in-vermoegensaufstellung-pro-konto/13855
Issue: https://forum.portfolio-performance.info/t/zeitlicher-verlauf-des-werts-im-moment-der-investition-vs-aktueller-wert/20326
Issue: https://forum.portfolio-performance.info/t/investiertes-kapital-nach-klassifikation-im-vermogensaufstellungsdiagramm/28110
Issue: https://forum.portfolio-performance.info/t/is-it-possible-to-make-a-custom-invested-capital-remove-bank-saving-from-chart/27167

@mierin12 mierin12 changed the title Allow a variety of data series for statement of assets V2 Add derived data series in Statement of Assets Chart Oct 12, 2024
@mierin12
Copy link
Contributor Author

mierin12 commented Oct 13, 2024

Little update to avoid the selection of securities for Interest/Interest charge and their accumulated data.
Capture d’écran 2024-10-14 005718
I was wondering about doing the same for Taxes and Taxonomies as it should always return 0 right ? But it is also a way for the users to see it by themselves.

Thinking of it, Interest/Interest charge and their accumulated data could also be simplified by removing Securities Account ? DataSeries.Type.PORTFOLIO_PLUS_ACCOUNT included, but DataSeries.Type.PORTFOLIO not included ? To further unclutter.-> Done

(it stays applicable to Securities Account + Reference Account)
@mierin12
Copy link
Contributor Author

mierin12 commented Oct 15, 2024

Hmmm I have just learned that for Security, tax are not included in the performance, and since the StatementOfAssetsSeriesBuilder is using the performance index, tax/tax accumulated for securities are 0 in the chart.
In the SecuritiesPerformanceView, total tax is shown, but I am not sure if this can easily be used a real data series.
What would be the expectation of the Statement of Assets chart on this topic ? Security's performance related taxes (0) or the real taxes? I guess it is better to be consistent with the other derived data series of security, so to keep 0, to stay consistent with the delta data series for example which is assuming tax =0.

@mierin12
Copy link
Contributor Author

Hello @buchen, what do you think ? I understand that this one may require a longer uninterrupted time slot for the review, otherwise, just in case you simply missed it.

@mierin12
Copy link
Contributor Author

Note : I did not manage to rebase locally to the latest master. But I managed to rebase after squashing the commits into a single one. For this single clean commit I tried to put both @shivampaw and me in co-author, I think this worked. if this is the correct way to do (?).

It will maybe be easier to work and review from this new branch:
master...mierin12:portfolio:pr-3754_rebased_derived_data_in_stat_of_assets_backup2

@mierin12 mierin12 closed this Dec 17, 2024
buchen added a commit that referenced this pull request Dec 20, 2024
Uses DerivedDataSeries to hold a reference to the underlying data
series (base) and the aspect to be shown (ClientDataSeries).

Issue: #3754
Issue: #4235
Issue: #4396
Co-authored-by: Shivam Paw <[email protected]>
Co-authored-by: mierin12 <[email protected]>
Co-authored-by: Andreas Buchen <[email protected]>
buchen added a commit that referenced this pull request Dec 20, 2024
Uses DerivedDataSeries to hold a reference to the underlying data
series (base) and the aspect to be shown (ClientDataSeries).

Issue: #3754
Issue: #4235
Issue: #4396
Co-authored-by: Shivam Paw <[email protected]>
Co-authored-by: mierin12 <[email protected]>
Co-authored-by: Andreas Buchen <[email protected]>
buchen added a commit that referenced this pull request Dec 21, 2024
Uses DerivedDataSeries to hold a reference to the underlying data
series (base) and the aspect to be shown (ClientDataSeries).

Issue: #3754
Issue: #4235
Issue: #4396
Co-authored-by: Shivam Paw <[email protected]>
Co-authored-by: mierin12 <[email protected]>
Co-authored-by: Andreas Buchen <[email protected]>
buchen added a commit that referenced this pull request Dec 21, 2024
Uses DerivedDataSeries to hold a reference to the underlying data
series (base) and the aspect to be shown (ClientDataSeries).

Issue: #3754
Issue: #4235
Issue: #4396
Co-authored-by: Shivam Paw <[email protected]>
Co-authored-by: mierin12 <[email protected]>
Co-authored-by: Andreas Buchen <[email protected]>
buchen added a commit that referenced this pull request Dec 21, 2024
Uses DerivedDataSeries to hold a reference to the underlying data
series (base) and the aspect to be shown (ClientDataSeries).

Issue: #3754
Issue: #4235
Issue: #4396
Co-authored-by: Shivam Paw <[email protected]>
Co-authored-by: mierin12 <[email protected]>
Co-authored-by: Andreas Buchen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
3 participants