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.

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

TradingRecord: support consecutive trades from same TradeType #755

Open
nimo23 opened this issue May 6, 2021 · 7 comments
Open

TradingRecord: support consecutive trades from same TradeType #755

nimo23 opened this issue May 6, 2021 · 7 comments
Labels
enhancement Issue describing or discussing an enhancement for this library

Comments

@nimo23
Copy link
Contributor

nimo23 commented May 6, 2021

This issue is for discussion (brainstorming) the best solution (implementation) to provide the ability to record consecutive trades from the same TradeType. After discussion, a PR should be created.

In short:

Redesign of backtesting logic (buy/sell at any index, drop the "just one open trade"-concept)

The actual (Base)TradingRecord (and all Criteria) does not support the following:

@Test
public void createConsecutiveEntries() {

    TradingRecord record = new BaseTradingRecord();

    // Currently, consecutive entries or exits are not supported. 
    // Only the first entry is recorded, the others are silently ignored:
   
    // we have a BUY-order executed in 4 trades (at different times with different quantites)
    record.enter(0);
    record.enter(1);
    record.enter(2);
    record.enter(3);

    // we have a SELL-order executed in 2 trades (at different times with different quantites)
    record.exit(4);
    record.exit(5);

    System.out.println(record);

    // analyze tradingRecord with "criteria"..
    // it seems that all criteria also needs to be adapted..
}

Additional informations:

@nimo23 nimo23 added the enhancement Issue describing or discussing an enhancement for this library label May 6, 2021
@MarcDahlem
Copy link
Contributor

I totally agree with you, that this would be a nice and usefull feature, especially for better backtesting.
A lot of people trade with strategies like "Market moved down --> Buy 25% deposit --> market moved down even more --> Buy again 25% deposit"...

But so far, the library's implementation is not ready to be reworked in such a big way.

  1. The enter(), close() and related functions like isOpen(), getLastTrade() or the complete idea of a "Position" are totally depending on the singular sequence of Enter and Exit.
  2. The caches, that are manually implemented in BaseBarSeries, CachedIndicator and more, are not made for handling such complex order relations. They even have some problems when the access to indices are executed in a "wrong" implementation order
  3. As you mentioned, all Criteria must be reworked to allow this feature.

All in all I would suggest to start with the small parts first, before we can implement arbitrary variations of entry and exit trades - especially the complex manual caching and index-behaviour should be reworked completely with more sophisticated structures like PriorityQueues, LinkedHashMap or even Caches and Sets.

@nimo23
Copy link
Contributor Author

nimo23 commented May 7, 2021

especially the complex manual caching and index-behaviour should be reworked completely with more sophisticated structures like PriorityQueues, LinkedHashMap or even Caches and Sets.

I agree. One of the main reason, why ArrayList was choosen: because it has the best overall performance (and can be accessed by indices). Well there are java built-in structures like (un)bounded queues/caches which would be worth to discuss..

@MarcDahlem
Copy link
Contributor

Normally I would say we use a TreeMap<Integer, T> for the cache and remove the map.firstEntry as soon as the cache is overfull.
The benefit of the TreeMap is the automatic sorting regarding the keys.

But there is the LinkedHashMap<Integer,T>, which automatically can remove the eldest/first indices: https://docs.oracle.com/javase/8/docs/api/java/util/LinkedHashMap.html#removeEldestEntry-java.util.Map.Entry-

I implemented this already here: https://github.com/MarcDahlem/ta4j/blob/feature/indicator_cache_enhancements/ta4j-core/src/main/java/org/ta4j/core/indicators/CachedIndicator.java#L40

The problem with LinkedHashMap is, that it does not sort by Key, so the value we remove could be a newer value...

@MarcDahlem
Copy link
Contributor

MarcDahlem commented May 7, 2021

Tada: #758
That replacement of the cache with TreeSet was really simple :-)

@miracle-kang
Copy link

Hi, @nimo23

I'm really interested in seeing this issue resolved, and I'm wondering if there have any updates.

@nimo23
Copy link
Contributor Author

nimo23 commented Jun 9, 2023

'm wondering if there have any updates.

No updates so far. Any updates, will be published here or in the form of a PR. Feel free to help :)

@wesssup
Copy link

wesssup commented Sep 2, 2023

This is the way forward because this is a normal way of doing transactions as you process more larger portfolio ... So I vote to implement it .... ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue describing or discussing an enhancement for this library
Projects
None yet
Development

No branches or pull requests

4 participants