Allow a variety of data series for statement of assets

[shivampaw]

https://forum.portfolio-performance.info/t/delta-diagramm-fur-ausgewahlte-aktien/6787/21

Adds the ability to add different values for the statement of assets charts

[shivampaw]

@buchen Hi! :) Any thoughts on this implementation?

[buchen]

Thanks for the contribution. I haven't had time to look into the details, but I like the idea of adding more options to the diagrams. Please give me a couple of more days as I am on the road this weekend.

[buchen]

Hey @shivampaw,

first: thank you very much for your contribution. I think the feature can be useful to all users. And regardless of my code comments, I think the approach to introduce a new type of DataSeries is the right one.

What we have to be careful is to not break backwards compatibility. Users have spend considerable amount of time to maintain their chart configuration. We do not want to break this and ask them to do it again.

Let's use the comments of the PR to discuss further.

Andreas.

[buchen]

The project files are actually check-in (common praxis with Eclipse) so that everybody gets the same configuration.

[shivampaw]

1e8f550 thanks!

[buchen]

This return statement prevents existing "common" series to be added.
We must not break existing diagram configurations.
Users have spent considerable time to setup their diagrams and they should not break.
Also, the upcoming mobile application is also supporting the existing modifiers.

Let's discuss in the comments of the pull request how to achieve backwards compatibility.

[shivampaw]

Perhaps we can add a DataSeries property for whether it's "visible"? And then for the returned DataSeriesSet for Statement of Assets, set it to false so the old ones don't show?

[buchen]

We cannot use parentObject.toString().
That can be a human readable name (for example in the case of classifications) which is not unique. For example, two taxonomies can have a category "Regions". That creates duplicate keys. It is often the case - also in my own file, that's why this code threw a DuplicateKeyException.

I think we need to use the same "identifier methods" that the DataSeries#uuidProvider uses.

[shivampaw]

Fixed this in 9566b57

[buchen]

This PR is using the "group" essentially as second label.
I understand that we need two labels (in the selection dialog the full label does not make sense, in the chart legend it is required to understand the data series).
I think we should make it then explicit and not the logic that the last element in the object array is a label.

[buchen]

About the new data series type (GroupedDataSeries):

I see two approaches:

1. Either we add a "modifier" to the DataSeries. The modifier is one of the applicable types of the ClientDataSeries (invested capital, taxes, ...). If it is not given, then we assume it is "total" (= the existing behavior)
2. or we introduce a new type (e.g. GroupedDataSeries). This type should "reference" one of the existing types (allows us to inherit the method to create identifier) and only include the "new" additional data series.

I have to put some more thought into, it but wanted to post it here to kick-start our discussion.

Pro & Cons of option 1 "modifier"

• Pro: the "instance" object remains typed per type (introducing a new data series type means that instance has again an object field with arbitrary types)
• Con: does not apply to SECURITY and SECURIT_BENCHMARK (but "group" is also a specific case already)

Pro & cons of option 2 "new data series type"

• Pro: possibly easier to extend
• Con: either requires migration of existing data series (including performance series) or applies only to new types (i.e. not TOTAL, but invested capital, etc.)

If we use option 2, then I think we could reference the "subtype" by one of the other types.
This way we the code in place to generate unique identifier based on the type of object.

public class GroupedDataSeries
{    
    private ClientDataSeries clientDataSeries;
    private DataSeries.Type subtype;
    private Object subtypeInstance;

    public String getId()
    {
        return subtype.buildUUID(subtypeInstance);
    }

Any thought?

To be honest, I have to sleep over this and maybe jot down some code for option 1 in order to better understand the problem.

[buchen]

About the UI:

I think for the first pull request it is okay to stick to the current "single tree" UI.
I would look something like

Taxonomies
|-- Asset Classes
|   |-- Equity
|   |   |- Total
|   |   \- Invested Capital
|   \-- Real Estate
|       |- Total
|       \- Invested Capital
|-- Asset Alllcation
...

But we could also do a UI with two selection options.

Taxonomies
|-- Asset Classes
|   |-- Equity
|   \-- Real Estate
|-- Asset Alllcation
...
-------------------------------
[X] total [ ] Invested Capital [ ] Fees ...

• Pro: it uncluttered the tree
• Open: how would the "multi selection" work. If the user selected more than one data series, does the second dialog also applies to all selected data series? Maybe this is a question for the forum....

[shivampaw]

About the new data series type (GroupedDataSeries):

I see two approaches:

1. Either we add a "modifier" to the DataSeries. The modifier is one of the applicable types of the ClientDataSeries (invested capital, taxes, ...). If it is not given, then we assume it is "total" (= the existing behavior)
2. or we introduce a new type (e.g. GroupedDataSeries). This type should "reference" one of the existing types (allows us to inherit the method to create identifier) and only include the "new" additional data series.

I have to put some more thought into, it but wanted to post it here to kick-start our discussion.

Pro & Cons of option 1 "modifier"

• Pro: the "instance" object remains typed per type (introducing a new data series type means that instance has again an object field with arbitrary types)
• Con: does not apply to SECURITY and SECURIT_BENCHMARK (but "group" is also a specific case already)

Pro & cons of option 2 "new data series type"

• Pro: possibly easier to extend
• Con: either requires migration of existing data series (including performance series) or applies only to new types (i.e. not TOTAL, but invested capital, etc.)

If we use option 2, then I think we could reference the "subtype" by one of the other types. This way we the code in place to generate unique identifier based on the type of object.

public class GroupedDataSeries
{    
    private ClientDataSeries clientDataSeries;
    private DataSeries.Type subtype;
    private Object subtypeInstance;

    public String getId()
    {
        return subtype.buildUUID(subtypeInstance);
    }

Any thought?

To be honest, I have to sleep over this and maybe jot down some code for option 1 in order to better understand the problem.

I don't 100% follow this but I think Option 2 makes sense - 9566b57 - and it means that the duplicate UUIDs are fixed. I don't understand the Con you mentioned as the way I see it, Performance charts are separate to the Statement of Asset charts so there probably shouldn't be an issue with us changing one implementation without changing the other?

[shivampaw]

About the UI:

I think for the first pull request it is okay to stick to the current "single tree" UI. I would look something like

Taxonomies
|-- Asset Classes
|   |-- Equity
|   |   |- Total
|   |   \- Invested Capital
|   \-- Real Estate
|       |- Total
|       \- Invested Capital
|-- Asset Alllcation
...

But we could also do a UI with two selection options.

Taxonomies
|-- Asset Classes
|   |-- Equity
|   \-- Real Estate
|-- Asset Alllcation
...
-------------------------------
[X] total [ ] Invested Capital [ ] Fees ...

• Pro: it uncluttered the tree
• Open: how would the "multi selection" work. If the user selected more than one data series, does the second dialog also applies to all selected data series? Maybe this is a question for the forum....

With multi selection it's still possible to cmd/shift+click on individual data series, do you think that's enough?

[buchen]

With multi selection it's still possible to cmd/shift+click on individual data series, do you think that's enough?

I think from Eclipse SWT I only get the list of selected elements. But no information which was the element selected last.

[shivampaw]

I think from Eclipse SWT I only get the list of selected elements. But no information which was the element selected last.

Sorry, I don't quite know what you mean :(

[mierin12]

Hello @shivampaw, hello @buchen

I have worked a bit on this topic, based on the work done on this PR. I think I have now reached something satisfying that I would like to propose in a PR. 😃
What is the good and correct way to include @shivampaw works ?
Should I somehow fetch the commits of this PR to keep their history & author and then add my commits after ? (how can I do it ?)
Or should I use the "Co-authored-by" line in my commits messages ?
I am still relatively new to git so I am really unsure about this kind of process.

Thanks !

[mierin12]

Hello @shivampaw, hello @buchen (sorry if this is too many tags, I am not sure if a new comment is sending a notification already)

A bit more details : you can find the final work in branch : master...mierin12:portfolio:derived_data_series_in_statement_of_assets_chart_squashed

I am waiting for your feedback on how to make reference to @shivampaw in this branch before doing the PR.

Similarities and differences with #3754:
Similarity:

• TYPE_PARENT and GroupedDataSeries kept. Some of its methods are slightly changed. So instance is indeed no longer a 100% Object type.

Differences:

• Data series selection dialog is distinct from the usual one -> less backward compatibility issue to deal with. To be careful about the DataSeriesSerializer load method modification where we have to combined back the usual data series and those new ones (so might create backward compatibility issue if I did incorrectly, I think it works well).
• In the new data series selection dialog, I am proposing a different tree order : [derived data]/Accounts & Portfolio/Account rather than Accounts & Portfolio/Account/[derived data]. I find it more intuitive but the order can be changed.
• Enum type done a bit differently to include also the boolean required to draw the data series either as a line or as a bar, and with an area or not.

Results:

Note on taxonomy labels:
For the selection label, two proposed possibilities:

• with fullPath that I kinda like but which is different than the existing behavior,
• with the same behavior as the existing data series selection

Then for its legend it is simplified with the simple name of the taxonomy, and the tooltip of legend shows the full path.

Multiselection topic:
No multiselection issue I believe. And the tree is already uncluterred by the use of the two selection dialogs.

[mierin12]

Ok I managed to fetch your pull request, rebase locally (correctly I hope) and add my modifs on top:
master...mierin12:portfolio:pr-3754-rebased_derived_data_in_statement_of_assets

phew. it is less readable but at least I managed to get both contributors in it.