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

BUG: Allow multiple names for vector indicators (#382) #980

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

Conversation

ivaigult
Copy link

@ivaigult ivaigult commented May 8, 2023

Fixes #382
Closes #385

Previously we only allowed one name per vector indicator:

def _my_indicator(open, close):
    return tuple(
        _my_indicator_one(open, close),
         _my_indicator_two(open, close),
    )

self.I(
    _my_indicator,
    # One name is used to describe two values
    name="My Indicator",
    self.data.Open,
    self.data.Close
)

Now, the user can supply two (or more) names to annotate each value individually. The names will be shown in the plot legend. The following is now valid:

self.I(
    _my_indicator,
    name=["My Indicator One", "My Indicator Two"],
    self.data.Open,
    self.data.Close
)

Copy link
Owner

@kernc kernc left a comment

Choose a reason for hiding this comment

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

Indeed, this looks good and is properly classified (bug). 😆

backtesting/_plotting.py Show resolved Hide resolved
backtesting/backtesting.py Outdated Show resolved Hide resolved
backtesting/backtesting.py Outdated Show resolved Hide resolved
backtesting/backtesting.py Outdated Show resolved Hide resolved
backtesting/_plotting.py Outdated Show resolved Hide resolved
@ivaigult ivaigult changed the title BUG: Allow multiple names for vector indicators (#382) [WIP] BUG: Allow multiple names for vector indicators (#382) May 9, 2023
@ivaigult ivaigult changed the title [WIP] BUG: Allow multiple names for vector indicators (#382) BUG: Allow multiple names for vector indicators (#382) May 9, 2023
@ivaigult
Copy link
Author

ivaigult commented May 9, 2023

Hi @kernc,

Thank you for the feedback! I think I addressed everything. Could you, please, resolve all conversations if it looks okay to you.

Thank you:slightly_smiling_face:

@ivaigult ivaigult requested a review from kernc May 9, 2023 21:30
backtesting/_plotting.py Outdated Show resolved Hide resolved
backtesting/_plotting.py Outdated Show resolved Hide resolved
@ivaigult
Copy link
Author

Hi @kernc,

I think it's all addressed, hope it's gtg now:crossed_fingers:

PS: similarly to #975, you need to approve workflow runs for me otherwise github won't run pylint and friends:slightly_smiling_face:

@ivaigult ivaigult requested a review from kernc May 14, 2023 19:57
Previously we only allowed one name per vector
indicator:

    def _my_indicator(open, close):
    	return tuple(
	    _my_indicator_one(open, close),
	    _my_indicator_two(open, close),
	)

    self.I(
        _my_indicator,
	# One name is used to describe two values
        name="My Indicator",
	self.data.Open,
	self.data.Close
    )

Now, the user can supply two (or more) names to annotate
each value individually. The names will be shown in the
plot legend. The following is now valid:

    self.I(
        _my_indicator,
	# Two names can now be passed
        name=["My Indicator One", "My Indicator Two"],
	self.data.Open,
	self.data.Close
    )

Co-authored-by: kernc <kerncece@gmail.com>
backtesting/_plotting.py Outdated Show resolved Hide resolved
@kernc
Copy link
Owner

kernc commented May 14, 2023

gtg

It is. The checks won't finish near green, though. Project docs build hinges on scikit-optimize being presently unmaintained and in a defunct state wrt other Python/numpy/sklearn stack. I'm not yet certain what to do about that.

@romanbsd
Copy link

romanbsd commented Sep 2, 2023

include the scikit-optimize, you actually need just a few files as far as I remember.

@ivaigult
Copy link
Author

Are there any obstacles that preventing merge of this?

scikit-optimize being presently unmaintained and in a defunct state

I understand it's an issue, but I appreciate this will be addressed separately in another PR?

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.

Allow setting legend names when multiple series values are used in an Indicator
3 participants