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

Add volume_mco api for volume custom colors #611

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

Conversation

alexpvpmindustry
Copy link
Contributor

new feature: add volume_mco such that it follows the mco colorscheme in marketcolor_overwrite

image

usage:

image

note:

  • works with all current marketcolor_overwrite inputs
  • marketcolor_overwrite must be valid
  • input is ignored if marketcolor_overwrite is None

@alexpvpmindustry
Copy link
Contributor Author

solves #320

@DanielGoldfarb
Copy link
Collaborator

DanielGoldfarb commented May 22, 2023

The code looks good, and I've tested on my and seems to work ok.

I'm just trying to think through a few things to decide whether or not to make some changes.

First I'm trying to remember why I wrote _make_updown_color_list() when I first implemented marketcolor_overrides, and I think that perhaps I had in mind eventually deprecating _updown_colors() (but I did not initially deprecated it because I had not yet implemented overrides for the volume colors, and I wanted to take things one step at a time).

There is also the issue of what I wrote at the bottom of the marketcolor_overrides notebook indicating that my intention for overriding volume colors was to only override volume colors when the override was a "marketcolor object" (the dict that is returned from mpf.make_marketcolors()) and when that marketcolor object included a "volume" attribute (for the up/down colors of the volume).

Comparing your implementation with this idea that I had posted at the bottom of the notebook, there are pros and cons to each: You implementation allows the volume overrides to be either a color-like object, or a marketcolor object. But it does not allow separate overrides for volume and candles. On the other hand, using only marketcolor objects to override volume (which would not require a separate volume_mco kwarg) allows one to override volume colors independently from overriding candle colors, but it does not allow the use of plain color-like objects.

@alexpvpmindustry let me know if you have any thoughts on this. Presently I am leaning towards your implementation because it is simpler (both as an implementation, and for a user to use). On the other hand, it would be nice, at some point in time, to deprecate _updown_colors() and have _make_updown_color_list() do all the work. That was the idea of passing in key. I am not sure why I didn't add use_prev_close as an argument when I wrote _make_updown_color_list(), but I am inclined to think that I wanted to keep it simple on the first pass and once I was confident everything was working I would then add that argument so that _make_updown_color_list() could also be used for volumes.

If you have an opinion on all of this, I am interested to hear it. And if not, that is fine too. Part of the reason for me writing this is just to have a record of my thoughts (which I probably should have written when I first implemented the overrides). I have no problem deciding to just merg this implementation, and worry about deprecating _updown_colors() some time in the future. My number one priority is always to not break any existing functionality.

@alexpvpmindustry
Copy link
Contributor Author

hello! hope u r doing well!
sorry i didnt reply earlier.
i don't have any opinions on these different implementations.
xD

@amircp
Copy link

amircp commented Mar 8, 2024

oh i was looking for this feature :(

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