Skip to content
This repository has been archived by the owner on Jun 8, 2023. It is now read-only.

✨ Split signals into triggers and guards #247

Open
wants to merge 23 commits into
base: development
Choose a base branch
from

Conversation

raftersvk
Copy link
Contributor

@raftersvk raftersvk commented Jan 21, 2022

Separation of signals into triggers and guards! 🎉
Also includes individual search space and parameters for weighted signals!

@Rikj000 Rikj000 changed the title Separation of signals into triggers and guards ✨ Split signals into triggers and guards Jan 30, 2022
@Rikj000 Rikj000 self-requested a review January 30, 2022 12:30
@Rikj000 Rikj000 added Feature - Enhancement Update or improvement to existing feature Feature - New New feature or pull request In Progress This is being worked on labels Jan 30, 2022
@Rikj000 Rikj000 added this to In Progress in MoniGoMani - Global Development Progress via automation Jan 30, 2022
@Rikj000 Rikj000 added this to the v1.0.0 milestone Jan 30, 2022
Copy link
Owner

@Rikj000 Rikj000 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR mate, some amazing work already done! 💯

I do have some comments on it though, please see them appended in my review,
looking forward to your reply 🙂

INSTALL_FOLDER_NAME="Freqtrade-MGM" # By default the folder will be created under the current working directory
MGM_REPO_URL="https://github.com/Rikj000/MoniGoMani.git"
MGM_BRANCH="development"
INSTALL_FOLDER_NAME="Freqtrade-MGM-Raftersvk" # By default the folder will be created under the current working directory
Copy link
Owner

Choose a reason for hiding this comment

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

This can remain until right before we're going to merge since it does allow for easily testing the PR,
but we should undo this when the PR is ready to merge!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you this is only a temporary change

@@ -17,15 +17,15 @@
"sell_trades_when_upwards": true
},
"weighted_signal_spaces": {
Copy link
Owner

Choose a reason for hiding this comment

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

mgm-config changes are very welcome! However they should go in a separate PR.

With a set of test results from before & after the mgm-config changes so that it becomes clear if they bring improvements & what they improve exactly 🙂

It also would be preferred to split up multiple mgm-config changes into multiple PRs,
so we truly have a clear overview of what improves what exactly! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I understand the idea to trace properly each change with a single PR.
These are the changes I made to my own mgm-config in order to get better profitable results, or to reduce/improve HO time without loosing much on quality.
not sure when I will find the time to "document" all those changes separately.
maybe somebody else can take this task ?

@@ -176,41 +231,87 @@ def do_populate_indicators(self, dataframe: DataFrame, metadata: dict) -> DataFr
# -------------------

# Parabolic SAR
dataframe['sar'] = ta.SAR(dataframe)
if (
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if I understand the need behind all these nested if-or blocks? 🤔
We didn't need them in the past, and to my knowledge we don't really need them now either?

However please do explain if we do need them!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea is to only populate the dataframe with signals which requires to be populated.
so if anyone wants to "disable" one of the default signal, he can remove the line from the initial buy/sell dictionary definition (or set the max = 0) and freqtrade will not loose any time in calculating and populating it

@@ -1248,6 +1269,9 @@ def _init_vars(cls, base_cls, space: str, parameter_name: str, parameter_min_val
if overrideable is True:
parameter_min_value = parameter_min_value + parameter_threshold
parameter_max_value = parameter_max_value - parameter_threshold
# Keep the original min value if search space is not big enough to restrict it
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if I agree with this 🤔
If a user configures a search space that will lead to impossible refinement values (e.g. min > max),
then we should abort the HO & warn him about it, instead of modifying the search spaces without his knowledge!

Copy link
Contributor Author

@raftersvk raftersvk Feb 2, 2022

Choose a reason for hiding this comment

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

I made these changes to be able to test only 1 signal as a trigger/guard which was impossible because of the way the min value worked with threshold and number of signals.
This change makes it possible without changing your original philosophy behind.

optimize = False
else:
optimize = True
# 1st HyperOpt Run or not overridable, just optimize without overrides
else:
optimize = True

# In case max <= min value no need to optimize, force default/max value to min
if max_value <= min_value:
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, if a user configures a search space that will lead to impossible refinement values (e.g. max < min),
then we should abort the HO & warn him about it, instead of modifying the search spaces without his knowledge!

Copy link
Contributor Author

@raftersvk raftersvk Feb 2, 2022

Choose a reason for hiding this comment

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

I came accross this because of my single signal test (to check the real quality of a given signal).
and I discovered that we were trying to set some freqtrade parameters with min = max values, which freqtrade doesn't accept (quite normal I would say), hence why I force the value and set optimize to false

@@ -1299,14 +1323,19 @@ def _init_vars(cls, base_cls, space: str, parameter_name: str, parameter_min_val

# 2nd HyperOpt Run: Apply Overrides where needed
if (parameter_value is not None) and (overrideable is True):
if default_value == override_parameter_min_value or default_value == override_parameter_max_value:
if (default_value == override_parameter_min_value or default_value == override_parameter_max_value) and ((override_parameter_max_value - override_parameter_min_value) > parameter_threshold):
Copy link
Owner

Choose a reason for hiding this comment

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

Could you elaborate this line a bit? 👀

signal_max_value = self.mgm_config['weighted_signal_spaces']['max_weighted_signal_value']
signal_threshold = self.mgm_config['weighted_signal_spaces']['search_threshold_weighted_signal_values']
if f'total_{space}_triggers_strength' not in dataframe.columns:
dataframe[f'total_{space}_triggers_strength'] = dataframe[f'total_{space}_guards_strength'] = 0
Copy link
Owner

Choose a reason for hiding this comment

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

Make sure to double check with a debugger that this doesn't link the f'total_{space}_triggers_strength' & f'total_{space}_guards_strength' columns together!

I'm not sure with a dataframe, but I know that Freqtrade/Parallel can behave weirdly like that sometimes
(e.g. if you define them like this, it might be so that if you edit one of the parameters, that the other one will be changed too, without that being our intention)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can simply make it two lines just to be sure and to avoid any risk

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Feature - Enhancement Update or improvement to existing feature Feature - New New feature or pull request In Progress This is being worked on
Development

Successfully merging this pull request may close these issues.

✨ Split signals into triggers and guards
2 participants