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.

Sign up for GitHub

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

Jump to bottom

Little changes in Criterions and order creation needed #23

Closed
team172011 opened this issue Sep 20, 2017 · 9 comments
Closed

Little changes in Criterions and order creation needed #23

team172011 opened this issue Sep 20, 2017 · 9 comments
Assignees
@team172011
Labels
bug Issue describing or discussing an bug in this library change Request for a change like renaming or refactoring enhancement Issue describing or discussing an enhancement for this library
Milestone
0.13

Comments

@team172011
Copy link
Member

Regarding: mdeverdelhan/ta4j-origins#194 (comment)

Ccob:

After further investigation there appears to be many of the Criterion classes all using the close price of the index the trade occurred instead of looking at the price of the order within the entry and exit trade. So for forward testing I'm guessing a whole set needs to be made or they need to be adjusted to look at close price or actual order price?

In my opinion it does not make much difference at the moment. But it needs to be fixed for the sake of accuracy
@team172011 team172011 added the bug Issue describing or discussing an bug in this library label Sep 20, 2017
@team172011 team172011 added this to the 0.10 milestone Sep 20, 2017
@team172011 team172011 self-assigned this Sep 20, 2017
@team172011 team172011 changed the title TotalProfitCriterion needs to use entry/exit prices TotalProfitCriterion should use entry/exit prices Sep 20, 2017
@team172011
Copy link
Member Author

It is possible to create an Order instance without hand out the price, but the index of time series where the order should be placed.

@team172011
Copy link
Member Author

And if we customize the criterions i would also include the amount of an order, with default = 1

@team172011 team172011 mentioned this issue Sep 23, 2017
Closed
@team172011 team172011 modified the milestones: 0.10, 0.11 Oct 20, 2017
@team172011 team172011 modified the milestone: 0.11 Oct 31, 2017
@team172011 team172011 added change Request for a change like renaming or refactoring enhancement Issue describing or discussing an enhancement for this library labels Nov 2, 2017
@team172011 team172011 reopened this Dec 4, 2017
@team172011 team172011 changed the title TotalProfitCriterion should use entry/exit prices Little changes in Criterions and order creation Dec 4, 2017
@team172011 team172011 changed the title Little changes in Criterions and order creation Little changes in Criterions and order creation needed Dec 4, 2017
@team172011 team172011 mentioned this issue Dec 4, 2017
Closed
team172011 added a commit that referenced this issue Dec 4, 2017
@team172011
Constructor of Order changed. TotalProfitCriterion can use order price
74b6d61 
Preparation for pending updates regarding Criterions see #23 #118

Changed the constructor of Order object:
    If creating a order via static buyAt() or sellAt() it must be
    delivered the corresponding time series,
    or a price and an amount. This should prevent to initialize an order with
    order price and amount of NaN, although the user wants to have close
    price (and default amount = 1) as order price.

    It is still possible to create an Order without time series and without
    price and amount, but it must happen explicit via
    sell/buyAt(index, NaN, NaN)

TotalProfitCriterion can use order price:
    The TotalProfitCriterion is the first criterion that has been
    updated regarding #23 #118.
    It now takes the order price if it is NaN it takes the close price of
    entry/exit indices
@team172011 team172011 mentioned this issue Dec 4, 2017
Merged
1 task
team172011 added a commit that referenced this issue Dec 4, 2017
@team172011
Constructor of Order changed. TotalProfitCriterion can use order price
73f47d6 
Preparation for pending updates regarding Criterions see #23 #118

Changed the constructor of Order object:
    If creating a order via static buyAt() or sellAt() it must be
    delivered the corresponding time series,
    or a price and an amount. This should prevent to initialize an order with
    order price and amount of NaN, although the user wants to have close
    price (and default amount = 1) as order price.

    It is still possible to create an Order without time series and without
    price and amount, but it must happen explicit via
    sell/buyAt(index, NaN, NaN)

TotalProfitCriterion can use order price:
    The TotalProfitCriterion is the first criterion that has been
    updated regarding #23 #118.
    It now takes the order price if it is NaN it takes the close price of
    entry/exit indices

Removed TraillingStopLossIndicator and Test
team172011 added a commit that referenced this issue Dec 4, 2017
@team172011
Constructor of Order changed. TotalProfitCriterion can use order price
9876fe7 
Preparation for pending updates regarding Criterions see #23 #118

Changed the constructor of Order object:
    If creating a order via static buyAt() or sellAt() it must be
    delivered the corresponding time series,
    or a price and an amount. This should prevent to initialize an order with
    order price and amount of NaN, although the user wants to have close
    price (and default amount = 1) as order price.

    It is still possible to create an Order without time series and without
    price and amount, but it must happen explicit via
    sell/buyAt(index, NaN, NaN)

TotalProfitCriterion can use order price:
    The TotalProfitCriterion is the first criterion that has been
    updated regarding #23 #118.
    It now takes the order price if it is NaN it takes the close price of
    entry/exit indices

Removed TraillingStopLossIndicator and Test
@mdeverdelhan
Copy link
Contributor

The analysis criteria are used in a backtesting phase. Basically, backtesting is executing a new strategy over past data.
In that phase I was thinking that we should not be polluted by any execution bias. I mean: of course the real order price is more precise. But if our strategy is based on the ClosePriceIndicator (for instance) the backtesting phase should be an ideal execution of it (i.e. if we react on close price moves, we should use the close price as our order price).

@team172011
Copy link
Member Author

I do not quiet understand you. If your strategy is based on close prices and you want to specify that your sell/buy price equals the underlying close price (currently the only possible case) you can do so further on. The only difference in #126 is, that the order price has to be initialized with this close price or another price

  • buyAt(i, series) -> order#price will be series#closePrice bar i.
  • buyAt(i, Decimal.TEN, DECIMAL.TWO) -> order#price will be ten and amount two

Otherwise it is confusing if we provide the Order api in the TradingRecord, but the criterions ignore completely its price and amount and just look at the close price. Because of that this (and some others) issue has been caused. Additional it gives the user the possibility to insert transaction costs or other influencing factors that will be considered from the criterions

@mdeverdelhan
Copy link
Contributor

OK, I understand. Sorry I did not check #126 before.

@team172011
Copy link
Member Author

Okay, thanks for your support

team172011 added a commit that referenced this issue Dec 9, 2017
@team172011
Constructor of Order changed. TotalProfitCriterion can use order price (
d079bdc 
#126)

* Constructor of Order changed. TotalProfitCriterion can use order price

Preparation for pending updates regarding Criterions see #23 #118

Changed the constructor of Order object:
    If creating a order via static buyAt() or sellAt() it must be
    delivered the corresponding time series,
    or a price and an amount. This should prevent to initialize an order with
    order price and amount of NaN, although the user wants to have close
    price (and default amount = 1) as order price.

    It is still possible to create an Order without time series and without
    price and amount, but it must happen explicit via
    sell/buyAt(index, NaN, NaN)

TotalProfitCriterion can use order price:
    The TotalProfitCriterion is the first criterion that has been
    updated regarding #23 #118.
    It now takes the order price if it is NaN it takes the close price of
    entry/exit indices

* Constructor of Order changed. TotalProfitCriterion can use order price

Preparation for pending updates regarding Criterions see #23 #118

Changed the constructor of Order object:
    If creating a order via static buyAt() or sellAt() it must be
    delivered the corresponding time series,
    or a price and an amount. This should prevent to initialize an order with
    order price and amount of NaN, although the user wants to have close
    price (and default amount = 1) as order price.

    It is still possible to create an Order without time series and without
    price and amount, but it must happen explicit via
    sell/buyAt(index, NaN, NaN)

TotalProfitCriterion can use order price:
    The TotalProfitCriterion is the first criterion that has been
    updated regarding #23 #118.
    It now takes the order price if it is NaN it takes the close price of
    entry/exit indices

Removed TraillingStopLossIndicator and Test

* Constructor of Order changed. TotalProfitCriterion can use order price

Preparation for pending updates regarding Criterions see #23 #118

Changed the constructor of Order object:
    If creating a order via static buyAt() or sellAt() it must be
    delivered the corresponding time series,
    or a price and an amount. This should prevent to initialize an order with
    order price and amount of NaN, although the user wants to have close
    price (and default amount = 1) as order price.

    It is still possible to create an Order without time series and without
    price and amount, but it must happen explicit via
    sell/buyAt(index, NaN, NaN)

TotalProfitCriterion can use order price:
    The TotalProfitCriterion is the first criterion that has been
    updated regarding #23 #118.
    It now takes the order price if it is NaN it takes the close price of
    entry/exit indices

Removed TraillingStopLossIndicator and Test
@team172011 team172011 modified the milestones: 0.11, 0.12 Jan 25, 2018
@team172011 team172011 removed this from the 0.12 milestone Feb 14, 2018
@team172011 team172011 added this to the 0.13 milestone Feb 14, 2018
@team172011 team172011 mentioned this issue Sep 15, 2019
Closed
@team172011 team172011 mentioned this issue Dec 25, 2020
Closed
@nimo23
Copy link
Contributor

nimo23 commented May 6, 2021

@team172011 this issue is 4 years open. (When) are you planning to work on this?

@team172011
Copy link
Member Author

I think this has been partially resolved by introducing the Position#getProfit and similar methods. So I think this can be closed as far as there are no further issues discovered by other users

@nimo23
Copy link
Contributor

nimo23 commented May 6, 2021 •
edited
Loading

@team172011 In general:

Would be nice to cleanup the remaining issues, those:

  • which are stale.
  • which are not needed to be resolved .
  • which are already resolved.
  • which are out of scope of this project.

Please take care of it and close all those issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@team172011 team172011

Labels
bug Issue describing or discussing an bug in this library change Request for a change like renaming or refactoring enhancement Issue describing or discussing an enhancement for this library
Projects
None yet
Milestone
0.13
Development

No branches or pull requests
3 participants
@mdeverdelhan @nimo23 @team172011