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

Trailing pct instead of ATR #223 #377

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

zlpatel
Copy link
Contributor

@zlpatel zlpatel commented Jun 13, 2021

No description provided.

@zlpatel zlpatel closed this Jun 13, 2021
@zlpatel zlpatel reopened this Jun 13, 2021
@zlpatel zlpatel changed the title Trailing pct instead of ATR kernc#223 Trailing pct instead of ATR #223 Jun 13, 2021
@Benouare
Copy link

Same as the other request.
On modification/feature/task = one commit.

backtesting/lib.py Outdated Show resolved Hide resolved
backtesting/lib.py Outdated Show resolved Hide resolved
Copy link

@Benouare Benouare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
I think that you should rebase your commit to have a clean historic, and change the propertie name.

super().init()

def set_trailing_sl(self, percentage: float = 5):
assert percentage > 0, "percentage must be greater than 0"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere else, the framework uses "pct" and "percent" to really mean rate (e.g. 5% == 0.05). I wonder if here shouldn't be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are you saying that a user should be inputting 0.05 instead of 5 and the code should use that value as is?

Comment on lines +482 to +489
index = len(self.data)-1
for trade in self.trades:
if trade.is_long:
trade.sl = max(trade.sl or -np.inf,
self.data.Close[index]*(1-self._sl_pct))
else:
trade.sl = min(trade.sl or np.inf,
self.data.Close[index]*(1+self._sl_pct))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
index = len(self.data)-1
for trade in self.trades:
if trade.is_long:
trade.sl = max(trade.sl or -np.inf,
self.data.Close[index]*(1-self._sl_pct))
else:
trade.sl = min(trade.sl or np.inf,
self.data.Close[index]*(1+self._sl_pct))
price = self.data.Close[-1]
for trade in self.trades:
if trade.is_long:
trade.sl = max(trade.sl or -np.inf, price*(1 - self._sl_pct))
else:
trade.sl = min(trade.sl or np.inf, price*(1 + self._sl_pct))

Copy link
Contributor Author

@zlpatel zlpatel Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I make this change, I want to let you know that I used index = len(self.data)-1 instead of -1 to be consistent with the TrailingStrategy code. I understand that in TrailingStrategy, we were using ATR which was computed differently and to get an absolute value for an index, we had to use len(self.data)-1, But I don't see any harm in using it here. Thoughts?

@zlpatel
Copy link
Contributor Author

zlpatel commented Sep 11, 2021

@kernc Have you had a chance to look at the questions that I asked?

@kernc kernc force-pushed the master branch 3 times, most recently from 7fd493d to e7981c7 Compare December 13, 2021 01:18
@kernc kernc force-pushed the master branch 7 times, most recently from 7e8513a to e28ac6a Compare November 28, 2022 03:08
@kernc kernc force-pushed the master branch 2 times, most recently from 60eff81 to 109c352 Compare November 28, 2022 22:33
@kernc kernc force-pushed the master branch 5 times, most recently from 428c361 to 0ce6cab Compare January 21, 2025 07:25
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.

3 participants