Position#getGrossReturn-addBase

[nimo23]

Fixes #.

Changes proposed in this pull request:

• added the parameter addBase to the following methods to be able to include or exlude the base from the result. The developer must now make a conscious decision whether to set addBase to true or false to avoid implicit bugs:

  • changed Position.getGrossReturn() to Position.getGrossReturn(boolean addBase)
  • changed Position.getGrossReturn(Num finalPrice) to Position.getGrossReturn(Num finalPrice, boolean addBase)
  • changed Position.getGrossReturn(BarSeries barSeries) to Position.getGrossReturn(BarSeries barSeries, boolean addBase)
  • changed Position.getGrossReturn(Num entryPrice, Num exitPrice) to Position.getGrossReturn(Num entryPrice, Num exitPrice, boolean addBase)

• With the above methods, we now make it explicit, whether a method (e.g. EnterAndHoldCriterion) is calculated with or without a basis - this will better avoid future bugs as the developer now must make a conscious choice whether to set addBase to true or false.

• With the above methods, we also solved a bug in ReturnCriterion#calculate(BarSeries series, Position position) as we are now able to include or exclude the base per position.

• added an entry with related ticket number(s) to the unreleased section of CHANGES.md

[TheCookieLab]

The fact that we include the base/principal in the return value is a choice of convention and not some kind of bug. Unless egregiously off the wall, conventions are neither right or wrong, they are simply a choice. "Correcting" a convention just because it may not have been the one you would have chosen is misguided and only leads to unnecessary complexity.

All frameworks and libraries have conventions, if a user is initially confused then the solution is simply to learn and adapt and not cater to every subjective preference out there. By doing the latter we establish precedent and potentially create a slippery slope of supporting all manners of representation variations.

Another potential problem with this change is negative returns. In a scenario where one wants to calculate aggregate returns of multiple investments, say, +10%, -2%, and +5% the current representation can correctly calculate it as 1.1 * 0.98 * 1.05 = 1.1254.
If we exclude the base then this becomes 0.1 * -0.02 * 0.05 = -0.0001 which is nonsensical.

[nimo23]

The fact that we include the base/principal in the return value is a choice of convention and not some kind of bug.

Without my fix, the user doesn't see at first glance whether a base is included or not. So my fix makes it easier to learn such conventions from the user's point of view. Something this important shouldn't be done under the hood.

Another potential problem with this change is negative returns. In a scenario where one wants to calculate aggregate returns of multiple investments, say, +10%, -2%, and +5% the current representation can correctly calculate it as 1.1 * 0.98 * 1.05 = 1.1254. If we exclude the base then this becomes 0.1 * -0.02 * 0.05 = -0.0001 which is nonsensical.

This is not relevant to this PR, as this PR does not exclude any base. It only gives the user of Position#getGrossReturn-methods the explicit choice. By the way, the Position#getGrossReturn-methods can be used for different purposes, not only for the purpose you describe in your "potential problem"..

"Correcting" a convention just because it may not have been the one you would have chosen is misguided and only leads to unnecessary complexity.

This fix does not correct any convention. It only gives the user a choice when using the Position#getGrossReturn-methods to include or exclude the base, nothing else (Btw, we also need these methods to fix a potential bug described above.) So I don't choose anything - with this PR, I'm allowing the user to choose - that's a big difference.

[nimo23]

But @TheCookieLab If you do not agree with this PR, please create a new PR with the following points:

• remove the base from ReturnCriterion and all other Criteria where the base is given as an optional property/parameter, otherwise, beside the "potential problems" you described, the bug (described above in this PR) still exists.

• improve the javadoc in all those methods Position#getGrossReturn-methods and all those Criteria-classes where the base is (implicitly) included to tell the user that the base is included. That would be the least to make it a little bit easier for the user to know our conventions and to know beforehand wether they should use these methods or criteria or implement their own by not including the base.

As long as there is no counter-PR, the above shortcomings and the bug remain. After getting this PR from you, we can decide which one is better (e.g. which PR gives more options for the user) - I guess my PR :)

[nimo23]

Another potential problem with this change is negative returns. In a scenario where one wants to calculate aggregate returns of multiple investments, say, +10%, -2%, and +5% the current representation can correctly calculate it as 1.1 * 0.98 * 1.05 = 1.1254. If we exclude the base then this becomes 0.1 * -0.02 * 0.05 = -0.0001 which is nonsensical.

@team172011 After this PR has been merged, I will create another PR that removes the addBase property from the ReturnCriterion, i.e. Including the base in ReturnCriterion should be mandatory (to avoid possible calculation errors, as described above). Users can use the new Position#getGrossReturn methods if they want to get the return without the base.