-
Notifications
You must be signed in to change notification settings - Fork 155
✨ Split signals
into triggers
and guards
#247
base: development
Are you sure you want to change the base?
✨ Split signals
into triggers
and guards
#247
Conversation
…s://github.com/raftersvk/MoniGoMani_raftersvk into separation-of-signals-into-triggers-and-guards
…s://github.com/raftersvk/MoniGoMani_raftersvk into separation-of-signals-into-triggers-and-guards
… search spaces mechanism
…nto separation-of-signals-into-triggers-and-guards
…earch space parameters
…iggers-and-guards
…s://github.com/raftersvk/MoniGoMani_raftersvk into separation-of-signals-into-triggers-and-guards
signals
into triggers
and guards
There was a problem hiding this 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 |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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": { |
There was a problem hiding this comment.
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! 😄
There was a problem hiding this comment.
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 ( |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
…s://github.com/raftersvk/MoniGoMani_raftersvk into separation-of-signals-into-triggers-and-guards
Separation of
signals
intotriggers
andguards
! 🎉Also includes individual search space and parameters for weighted signals!