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

Pyramid Trading - Scaling into a position - DCA Trading possible > Possible way #1113

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wesssup
Copy link

@wesssup wesssup commented Sep 10, 2023

Possible approach :#1106

Allowing Pyramid Trading - Scaling into a position - DCA Trading possible > Possible way
Still work to be done !!!!

  • Code only shows impact on de base classes
  • isPyramidTrading and Depth configuration should be moved to another way ....
  • Unit tests for pyramid trading are missing
  • Due to some base class changes - existing unit testing are failing, but is expected since the operate functions ,which are tested are impacted.

--> Not for MERGE on head branch ! -> Dev still needed see ticket for more info

@nimo23
Copy link
Contributor

nimo23 commented Sep 10, 2023

Still work to be done !!!!

No work should be done in this PR anymore, because the basis and the basic assumptions (about how to solve this) is simply not good. Everything would have to be tackled completely differently in order to solve this problem, so this PR cannot be improved in this way either.

I do not agree with this PR. It is nothing else than a workaround which pollutes a lot of our base classes only because of one concept. It is by far not a clean solution and would worsen the condition of the lib, since one concept was sloppily crammed into other concepts. @wesssup If it works for you, then leave it in your branch. I will not agree to put this into this lib. It would be far better to provide a clean solution to this problem as suggested here: #1054 (comment). If #1054 was done, then this "pyramid trading" would end up being nothing more than a criterion type indicator (called PyramidCriterion). That would be the only clean solution.

This PR is nothing more than a workaround and is far from a clean and future-proof solution - it's a typical hack and pollutes the lib.

@wesssup
Copy link
Author

wesssup commented Sep 11, 2023

--> Pls read ticket #1106 I already provided an answer ...

I don't agree on the hack part , as you will need to change the core logic, I don't see how you can move otherwise from allowing only one position to multiple ... needed for scaling into a bigger transaction.

But true, as I mentioned above ... there's still work to be done ... BUT we did some tests in comparison with Trading view Pine back testing ... and we got the same results with this code ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants