fix moving average entry price in Trades when security is shared between several portfolio

[mierin12]

Issue : https://forum.portfolio-performance.info/t/gleitender-durchschnitt-zwei-depots-trade/32840
Hello,

There was an issue on moving average entry value calculation for Trades when the same security is shared between several portfolio. The same entry value was considered, while each portfolio has their own transactions : the two tests testMovingAverageTradesSeveralPortfolio and testMovingAverageTradesSeveralPortfolio2 added failed on the current build.
This PR proposes to fix it.

In a second commit, I have added also some support for Short/Long for moving average. To support Short/long, the method to calculate moving average entry value is changed (no ClientTransactionFilter anymore), as it relied on LazySecurityPerformanceSnapshot based on CostCalculation which I think does not support short. So, same as for the FIFO entry value of trade, I think we can calculate entry value directly in a way which allows for short support rather smoothly.

[mierin12]

(one note to remember for latter, I think there is still a discrepancy between CostCalculation moving average entry value and Trade moving average entry value in case of transfers. I am not sure which is one is the correct one, but I think CostCalculation does not take them into account (but does for FIFO) while with Trade Collector, transfer are taken into account also for mov average calc.
Anyway this behavior is not impacted by this PR, but just to remember in the future if we want to address it.)

[buchen]

Thanks @mierin12.

just to make sure I understand your change:

At the moment, the moving average is calculated against the moving average of the whole file ("globally") and not only against the portfolio in which the instrument is held. First, I understand you propose to calculate the moving average per portfolio instead.

Second, you propose a simpler implementation: instead of filtering the whole client, your implementation is collecting all relevant transactions during the TradeCollector. Then the calculation "just" has to iterate over those transactions to get the moving average.

Is that a fair understanding?

My thinking is:

At least when looking at the CostCalculation class, the moving average is calculated globally (across the whole file). Would we have to change something there as well? Or is it handled by existing ClientFilters? (Say: when selecting on portfolio in the "grouped accounts", the "statement of assets" in the bottom pane will filter and also only show the moving average per portfolio).

Let me also check with the person that made me add this feature. Some tax authorities calculate taxes for a trade on the moving average. Then the FIFO calculation in PP is not helpful and that's why I added profit loss per moving average. Now the question is what tax authorities do: do they separate by bank (=portfolio) as well?

The second point makes sense. I tried to reuse the movingAverage calculation but that overcomplicates things by introducing this not so simple client filter.

one note to remember for latter, I think there is still a discrepancy between CostCalculation moving average entry value and Trade moving average entry value in case of transfers.

I know. The Trade is "better" in the way that it handles (some cases of) short sales. The CostCalculation still struggles from a not so good sorting heuristic if transactions have or have not a time (besides the date). It requires continuous time that I currently not have in order to get my head around it.

[mierin12]

At the moment, the moving average is calculated against the moving average of the whole file ("globally") and not only against the portfolio in which the instrument is held. First, I understand you propose to calculate the moving average per portfolio instead.

Second, you propose a simpler implementation: instead of filtering the whole client, your implementation is collecting all relevant transactions during the TradeCollector. Then the calculation "just" has to iterate over those transactions to get the moving average.

Is that a fair understanding?

Yes, exactly, 100%. If I buy an ETF/stock in bank A and then the same ETF/stock in a bank B, their entry prices (be it FIFO or mov average way, are distinct.).
Sorry usually I give more details in my PR, here is a comment on the forum about it :
Issue : https://forum.portfolio-performance.info/t/gleitender-durchschnitt-zwei-depots-trade/32840

And attached an example file :

test fix mov avg.xml

Portfolio 1 : 1 buy for 500 € (5 * 100 €)
Portfolio 2 : 1 buy for 1000 € (5 * 200 €)

Transaction of Portfolio 1 & 2

There isn't even any sell, so we should not have difference FIFO vs mov average, but only the FIFO trades does show correctly the entry price distinct for each portfolio. The Trade mov average entry price is computed as (1000+500)/2=750€ which is incorrect for me.

Result in Trades which for me is incorrect for moving average

After the PR :

(and on the picture we can see the impact on a simple short trade test that I also included in the file.)

There is no need to change CostCalculation over this, the filter already takes care of it. Portfolio 1 purchase price is not affected by transaction of Portfolio 2 and vice versa :

[mierin12]

Maybe @MrTempiko from #4503 could advise on this ?

[MrTempiko]

Now the question is what tax authorities do: do they separate by bank (=portfolio) as well?

I can only speak for Austria, but here the moving average is always within a portfolio (acc. to the income tax act).
So yes, tax authorities do separate by bank or, to be even more exact, by portfolio.

[mierin12]

Thanks Mr Tempiko ! For me, for France too the entry price/entry price per share is per portfolio.

I have updated my post above to try to be more clear.

For info, this was the first way I tried to do, by still using LazySecurityPerformanceSnapshot :
master...mierin12:portfolio:SKIP/fix_acquisition_price_in_Trades_for_moving_average_method

Basically same idea, we rely on TradeCollector (which is per portfolio, rather than ClientFilterTransaction which was whole portfolio) to collect the relevant transactions and then compute over it. But I was stuck by the new Short Sell feature. And I believe I did get some issue with .subList(0, transactionsMovingAverage.size() - 1) in some cases.

Hence, the way proposed in the PR, without ClientFilterTransaction nor LazySecurityPerformanceSnapshot. But I do not see it as an issue, for FIFO also it is computed without LazySecurityPerformanceSnapshot

        this.entryValue = transactions.stream() //
                        .filter(t -> t.getTransaction().getType().isPurchase() == isLong)
                        .map(t -> t.getTransaction().getMonetaryAmount()
                                        .with(converter.at(t.getTransaction().getDateTime())))
                        .collect(MoneyCollectors.sum(converter.getTermCurrency()));