[BUG] ProfitLossCriterion: Javadoc says "excludes trading costs" but code uses trading cost

[Cashmeiser]

I have checked existing issues and wiki

• I could not find similar issues
• I could not find a solution in the wiki or faq section

Describe the bug
Javadoc in ProfitLossCriterion says "Net Profit and loss criterion (absolute PnL, excludes trading costs)."
The code uses: position.getProfit();
and that code is:

    /**
     * Calculates the net profit of the position if it is closed. The net profit
     * **includes any trading costs.**
     *
     * @return the profit or loss of the position
     */
    public Num getProfit() {
        if (isOpened()) {
            return zero();
        } else {
            return getGrossProfit(exit.getPricePerAsset()).minus(**getPositionCost()**);
        }
    }

So, what is expected from ProfitLossCriterion? With Costs as code, or exclude cost as javadoc says?

Expected behavior
Consistency between Javadoc and Implementation

[TheCookieLab]

Good catch, do you want to create a PR to resolve this issue? Thanks.

…

On Wed, Feb 14, 2024 at 3:38 PM Cashmeiser ***@***.***> wrote: *I have checked existing issues and wiki* - I could not find similar issues <https://github.com/ta4j/ta4j/issues?utf8=%E2%9C%93&q=> - I could not find a solution in the wiki <https://ta4j.github.io/ta4j-wiki/> or faq section <https://ta4j.github.io/ta4j-wiki/FAQ.html> *Describe the bug* Javadoc <https://github.com/ta4j/ta4j/blob/master/ta4j-core/src/main/java/org/ta4j/core/criteria/pnl/ProfitLossCriterion.java#L33> in ProfitLossCriterion says "Net Profit and loss criterion (absolute PnL, *excludes trading costs*)." The code uses: position.getProfit(); and that code is <https://github.com/ta4j/ta4j/blob/master/ta4j-core/src/main/java/org/ta4j/core/Position.java#L236> : /** * Calculates the net profit of the position if it is closed. The net profit * **includes any trading costs.** * * @return the profit or loss of the position */ public Num getProfit() { if (isOpened()) { return zero(); } else { return getGrossProfit(exit.getPricePerAsset()).minus(**getPositionCost()**); } } So, what is expected from ProfitLossCriterion? With Costs as code, or exclude cost as javadoc says? *Expected behavior* Consistency between Javadoc and Implementation — Reply to this email directly, view it on GitHub <#1130>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQJ6FOIP5NA2OTYQTO3VPLYTUOERAVCNFSM6AAAAABDJBDRPWVHI2DSMVQWIX3LMV43ASLTON2WKOZSGEZTKMJWGQ2DMNI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Cashmeiser]

Yes, I have a fix (like other criteria) with a constructor with boolean excludeCosts and a default one with excludeCost=false.
There is more job with fixing ReturnCriterion which also is inconsistence with java doc vs actual code. But this fix enforces some new methods in Positions as well to calculate Return with costs. Missing getNetGrossReturn methods.

But to do this, I need Permission to push a repo. Can you give it to me?

[TheCookieLab]

Typically contributing to open source projects is done via pull requests. Please see the contribution guide here: https://ta4j.github.io/ta4j-wiki/How-to-contribute

As for the implementation, to be consistent with the rest of the org.ta4j.core.criteria.pnl package it's preferable to make separate classes for "Gross" and "Net" profit/loss rather than using a boolean param.

[Cashmeiser]

I get permission denied when I try to push my feature-branch:

> git push --set-upstream origin feature/1130-improve-pnl-criteria
info: please complete authentication in your browser...
remote: Permission to ta4j/ta4j.git denied to Cashmeiser.
fatal: unable to access 'https://github.com/ta4j/ta4j.git/': The requested URL returned error: 403

About the implementation, I took the boolean param from the other existing PnL: https://github.com/ta4j/ta4j/blob/master/ta4j-core/src/main/java/org/ta4j/core/criteria/pnl/ProfitCriterion.java#L42

[Cashmeiser]

Maybe its on my side then. I had cloned the repo, created a feature-branch but couldn't push.
Tried to start over with the ta4j-wiki but then I couldn't even clone it:

git clone https://github.com/Cashmeiser/ta4j.git
Cloning into 'ta4j'...
info: please complete authentication in your browser...
remote: Repository not found.