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

Set single candle color #451

Merged
merged 13 commits into from
Dec 14, 2021
Merged

Conversation

AurumnPegasus
Copy link
Contributor

Hey @DanielGoldfarb ,
I have completed and tested (from my side) the code for #378 as we had discussed.
I have:

  • Kept a check such that length of open/close/high/low values must be equal to list of color
  • Allowed user to enter either a string as colour or marketcolor
  • Did the same for ohlc bars as well

If I must make some improvements let me know! Hope it is satisfactory 😄

@DanielGoldfarb
Copy link
Collaborator

I've started testing and going through the code. There will be probably a few changes. Overall it looks good. I have found that when passing marketcolor dicts it does not always work as expected. Will be looking into that tomorrow. Will send a list of recommended changes once I have finished all testing. Thank you again, very much, for contributing.

Copy link
Collaborator

@DanielGoldfarb DanielGoldfarb left a comment

Choose a reason for hiding this comment

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

First pass, some comments and minor changes to consider (see within the diff below).

Also, I wrote some test code based on the example in Issue #378 which you may want to use for some testing:

import pandas as pd
import mplfinance as mpf

df = pd.read_csv('data/SP500_NOV2019_Hist.csv',index_col=0,parse_dates=True)

clist   = [None]*len(df)
cseries = pd.Series(clist,index=df.index)

cseries.at['2019-11-08'] = 'yellow'
cseries.at['2019-11-15'] = 'yellow'
cseries.at['2019-11-19'] = 'yellow'

mpf.plot(df,type='candle',colors=list(cseries.array),style='yahoo',block=False)

mc = mpf.make_marketcolors(base_mpf_style='default',up='yellow',down='yellow')
cseries = pd.Series(clist,index=df.index)
cseries.at['2019-11-08'] = mc
cseries.at['2019-11-15'] = mc
cseries.at['2019-11-19'] = mc

mpf.plot(df,type='candle',colors=list(cseries.array),style='yahoo')

The first mpf.plot() above, should produce three completely yellow candles.

The second mpf.plot() should produce three candles with yellow bodies but with black edges and wicks similar to the example in the Issue.


(Note: the black edges and wicks come from the style 'default' that was specified in the call to make_marketcolors(); Even if unspecified, 'default' would still have be used for the edges and wicks because the call to make_marketcolors() only specifies up and down).

 

src/mplfinance/_arg_validators.py Outdated Show resolved Hide resolved
src/mplfinance/_arg_validators.py Outdated Show resolved Hide resolved
src/mplfinance/_utils.py Outdated Show resolved Hide resolved
src/mplfinance/_utils.py Outdated Show resolved Hide resolved
src/mplfinance/_utils.py Outdated Show resolved Hide resolved
src/mplfinance/plotting.py Outdated Show resolved Hide resolved
@AurumnPegasus
Copy link
Contributor Author

Hey @DanielGoldfarb,
I have read and understood your comments related to my code, and will make the respected changes within the next week! Should I also upload a notebook / make changes in existing notebook on how to use this feature?

@DanielGoldfarb
Copy link
Collaborator

Shivansh,
Sorry I've been out of touch for over a week ... been working on some other projects. Regarding my confusion and your comments:

Right so the issue here was that by default if I am say sending marketcolor where I have specified say upcolor, downcolor only, then everything else by default is given a value 'k' , which resulted in it being black. But from what I saw and understood by default if I have not specified a particular colour it should take colour of the default theme.

It takes care of very rare cases where I might send marketcolor with say only upcolor specified but not downcolor, in which case it should become same as background color and not black as it was becoming

Let me explain my thinking on the matter: From the user's perspective, the user can specify override_marketcolors= as a list (equal in length to the ohlc dataframe) where some elements of this list are None and some elements are either a string type color specifier or an mplfinance marketcolor object (dict).

My thinking was originally (will clarify below why I say "originally" here) that

  • if the user specifies a string type color specifier, then user wants the entire candle (body, edge, and wick) to be the one specified color at that location.
  • If, however, the user specifies a marketcolor dict for that candle, then it is 100% the user's repsonsibility to ensure that all the colors of that candle marketcolor object (body, edge, and wick) are precisely what the user wants, and it is the user's responsibility to ensure that none of those colors will conflict or blend-in with any other colors on the chart.

So there should be no code that manipulates any of the marketcolor attributes, other than the code that converts a string type color specifier to a marketcolor dict where all the attributes are the same (specified) color.

Note: when the user creates marketcolor objects (using mpf.make_marketcolors()) then, if the user chooses to specify only some of the colors (for example, just "up" and "down" as you mentioned), then make_marketcolors() will use the "base_mpf_style" to set the other marketcolor attributes. It is the user's responsibility to make sure the kwarg base_mpf_style= (in the call to mpf.make_marketcolors()) is set to the appropriate mpf style. Typically this would be the same style the user specified when calling mpf.plot().

If the user forgot to specify base_mpf_style= when calling mpf.make_marketcolors(), then it base_mpf_style would be set to "default" which may result in candles not appearing as expected, if in fact the user had specified a different style when calling mpf.plot(). Please correct me if I am wrong, but I believe this is the case you are trying to circumvent with the code that was confusing me. If so ...

I still think we should not have such code. However now, based on your comments, I am now thinking that we may either keep my original thinking and push the entire responsibility back on the user to get the marketcolors correct, or we recognize that specifying only the candle body may be a common thing to do, and we might create a relatively easy way for the user to do this such that the user need only specify the color, or up/down, and the rest of the marketcolor attributes come from the style that was passed into mpf.plot() without the need for the user to call mpf.make_marketcolors() and specify the same style again in yet another place.

I hope this is making sense. Let me know if you have questions. I have some ideas as to how we might implement this last idea (giving the user an easy way to specify up/down and use the remaining attributes from the plot's marketcolors). As I am writing this, however, I am also considering staying with the original idea of putting the entire responsibility on the user to call mpf.make_marketcolors() and specify the correct base_mpf_style. It will keep the code simpler and allow us to get this feature out quicker, and we can always later add some code that figures out, for the user, the remaining marketcolor attributes (based on the plot style) and allows the user to avoid calling make_marketcolors().

Thanks. --Daniel

@DanielGoldfarb
Copy link
Collaborator

@AurumnPegasus
Shivansh, after a delay due to other projects, I am now in the process of eliminating the backlog of Pull Requests, and I hope to get to this one within a day or two. If you have any changes you have made, as you mentioned above, please push them to your fork (so they will flow through to this pull request). Otherwise, when I get to this in another day or two I will finish up the changes myself. Please let me know. Thank you very much! --Daniel

@AurumnPegasus
Copy link
Contributor Author

hey @DanielGoldfarb I am so sorry for the delay, the last month of semester really is awful with respect to deadlines. If you can, please give me another day to complete this, I will have it done by 4th dec

@DanielGoldfarb
Copy link
Collaborator

no problem. thank you.

@AurumnPegasus
Copy link
Contributor Author

Hey @DanielGoldfarb,
I have made the changes as requested (I am still unsure about the issue mentioned in here so I have created a trial.py in src which initialises random dataset of candle sticks and gives them different colours, check them out once and let me know if that is what you actually meant?

Again, sorry for the delay, hope the code is acceptable now!

@DanielGoldfarb
Copy link
Collaborator

@AurumnPegasus
Shivansh, Thank you. The trial.py file is helpful. I plan to play around with the code for a bit; try some experiements to be sure I understand how it is working. In the meantime I have a couple of questions:

  1. I assume you closely looked at the results of trial.py and you are happy with the way they look. Is this a correct assumption?
  2. Are you able to perhaps give me a real world example of how you expect that you will use this feature? Even if it uses made up data; just so that I get a more clear idea of how you envision using this new feature.

Thank you. --Daniel

@AurumnPegasus
Copy link
Contributor Author

@DanielGoldfarb

  1. Yup! I tried all the possible combinations and it seems really good!
  2. So the reason I was looking for this feature in particular was because me and my friends were learning about candle chart indicators and trying to code some algos up for trading! different candles. Having various colours for it was just helpful in finding where exactly they occur (and focusing on stock patterns around them)

@DanielGoldfarb
Copy link
Collaborator

@AurumnPegasus
Shivansh,
Thanks for the information; and again thank you for contributing.

I've been playing around with the code and it seems to be working well. I do believe that we can make the algorithm simpler (and thereby easier to maintain). That said, I should point out that, when I started actually playing with the code, I realized that it is not so trivial to override the marketcolors on a per-candle basis. I am therefore even more grateful for your contribution. Thank you so much!

In order to simplify the code, I had to completely rewrite _updown_colors(). I've called the new version _make_updown_color_list(). Several of the arguments are different, as well as the algorithm. You will see this when I update the code for this Pull Request. In the next day or two I will update this Pull Request by updating your fork (from which this Pull Request was generated). This will allow you to review and test my changes. If there are any problems with it, then I can always back the changes out.

In the meantime, I need to regression test thoroughly to make sure we have not broken any of the established features of mplfinance. I also want to set up some regression tests and examples specifically for this new feature. This is working now for candlesticks. I would like to be able to make it also work for hollow candles, for ohlc bars, and possibly also for renko and point and figure charts. Stay tuned.

@DanielGoldfarb
Copy link
Collaborator

DanielGoldfarb commented Dec 9, 2021

OK. I have updated AurumnPegasus/mplfinance and thereby also this PR. A few points to note:

  • I have moved trial.py from src to examples/scratch_pad as this is where files such as this should be placed in the repository (i.e. files that involve testing and experiments and examples used for development purposes). At the same time, I renamed the file to pr451_test.py in order to more specifically describe its purpose. I also made some modifications, most significantly I ran the part of the script that generates random data and then saved that data as pr451data.csv so that now, each time the script runs, it can use the same data. This way we can compare new code to old code when making changes, knowing that the data has not changed. There is also a jupyter notebook pr451_testing.ipynb which serves more-or-less the same purpose as pr451_test.py (formerly known as trial.py).

  • I added a tutorial to the examples folder: examples/marketcolor_overrides.ipynb. Please go through the tutorial and let me know if there is anything in there that is not clear. Thank you.

  • Please test this new code against your use cases and let me know if anything is not working. Note that I have significantly modified the code, hopefully simplifying it, and I removed blocks of code like this:

                 if candle_up == 'w':
                      candle_up = marketcolors['candle']['up']
                  if candle_down == 'k':
                      candle_down = marketcolors['candle']['down']
                  if edge_up == 'k':
                      edge_up = candle_up
                  if edge_down == 'k':
                      edge_down = candle_down
                  if wick_up == 'k':
                      wick_up = candle_up
                  if wick_down == 'k':
                      wick_down = candle_down
  • I discovered that my original suggestion, to convert single-color overrides into marketcolor objects, was completely unnecessary, so I have removed that code as well. Please let me know if you see any problem with this. Thank you.

@AurumnPegasus
Copy link
Contributor Author

Hey @DanielGoldfarb ,

I went through the tutorial notebook, and that looks really good! I also went through the changes you made in the code and liked them (really helps me improve my own coding). It works well from my use case perspective.

Thanks!

@DanielGoldfarb
Copy link
Collaborator

@AurumnPegasus
Shivansh, Thank you! I will merge the code and deploy to Pypi sometime in the next few days. Just need to make a couple of small corrections to the tutorial. All the best.

@DanielGoldfarb DanielGoldfarb merged commit 1bf18a6 into matplotlib:master Dec 14, 2021
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.

2 participants