Added labels for make_addplot function (lines,bars,scatter). Missing …for _mscatter function. #277
Conversation
…for _mscatter function.
There was a problem hiding this comment.
- See also individual comments in the code below.
Here is a more general comment:
I did some testing with the addplot notebook, adding a labels= kwarg to several of the examples in that notebook. It appears that Axes.legend(labels=labels) requires that labels be an iterable (list or tuple) of labels. If I pass a single string, it gets treated as a list of 1 character labels (since a string object is iterable).
However I do like the idea of allowing mplfinance users to pass a single string (rather than forcing them to pass a len=1 tuple such as labels=('my label',), which I had to do in order to get the code to work as expected for the case of a single label).
Therefore I would suggest that, perhaps somewhere near line 772 (aptype = apdict['type']) we could insert:
labels=apdict['labels']
if isinstance(labels,str):
labels = [labels] # convert to listThen in all subsequent code below that, only need to use the variable labels (and no need to again reference apdict['labels']).
| if config['title'] is not None: | ||
| if config['tight_layout']: | ||
| # IMPORTANT: 0.89 is based on the top of the top panel | ||
| # being at 0.18+0.7 = 0.88. See _panels.py | ||
| # If the value changes there, then it needs to change here. | ||
| title_kwargs = dict(size='x-large',weight='semibold', va='bottom', y=0.89) |
There was a problem hiding this comment.
It appears that this section was removed. I'm guessing that you did not do this on purpose, but rather made your changes to a version of the repository that was cloned before August 16, 2020 (when this enhancement was merged into the repository). A more recent version of the repository should be merged into this pull request (or, alternatively, these changes can be applied to a more recent clone of the repository, and then push that clone into the fork that is the basis for this PR).
If you have any questions about this, please let me know
| def _label_validator(label_value): | ||
| if isinstance(label_value,str): | ||
| return True | ||
| elif not isinstance(label_value,tuple) and not isinstance(label_value,list): |
There was a problem hiding this comment.
This can be simplified:
elif not isinstance(label_value,tuple) and not isinstance(label_value,list):becomes
elif not isinstance(label_value,(tuple,list)):| else: | ||
| ax.scatter(xdates,ydata,s=size,marker=mark,color=color,alpha=alpha) | ||
| if apdict["labels"] is not None: |
There was a problem hiding this comment.
Since apdict["labels"] was assigned to labels above, then labels should be used here and below.
| @@ -787,22 +775,29 @@ def _addplot_columns(panid,panels,ydata,apdict,xdates,config): | |||
| mark = apdict['marker'] | |||
| color = apdict['color'] | |||
| alpha = apdict['alpha'] | |||
| labels = apdict["labels"] | |||
There was a problem hiding this comment.
Use single quotes, apdict['labels'] instead of apdict["labels"], only for the sake of being consistent with existing/surrounding code style.
This is a good, general practice: Whenever modifying any existing code, keep to the style of the existing code (assuming that code has a consistent style to begin with). (If existing style is already inconsistent, then consider modifying the code to be consistent).
|
Status update: I am presently reviewing all outstanding pull requests. did some experimenting with labels in general beyond just what is in this PR (see https://github.com/DanielGoldfarb/mplfinance/tree/ppseverin). When we finish this enhancement for labels I want to be able to support not just |
hello, thanks for all the work so far! |
|
@alexpvpmindustry Thanks for your interesting in, and expression of gratitude for, mplfinance. Much appreciated! Here's the status: I am solidly involved in another project at this time, and in preparing for JupyterCon (hope to see you there) so I will be giving minimal attention to mplfinance at least until towards the end of May. That said, if you read through the conversion above, you can see where I have made several suggestions regarding the code. I've also pointed out where I think we should be going with labels, annotations, and legends, specifically with regard to for which aspects of mplfinance these things should be supported. (until now, there appears to be no movement on my suggestions). If you would like to contribute, along the lines of the discussion and requests above, then I will be happy to review any contributions. (Keep in mind that it has been a while since this PR was first entered, and the trunk may have since migrated somewhat; so a fresh clone may be in order). Alternatively you can wait for me to make these changes, in which case it will most likely be the middle of June or later before it will be done. As mentioned above, a core part of this particular enhancement is complete testing to ensure no existing functionality has been broken. |
|
@DanielGoldfarb i see! sorry, i wont be available for the jupytercon, sounds exciting tho. |
|
seems like @ppseverin has deleted his repo, ill make a new PR |
|
This was implemented via #605 |
Its missing for the _mscatter function. I didn't understood very well the function (maybe if I spent more time on it I could). Tested on jupyter lab and worked fine. Hope this is useful for the community. :)