-
Notifications
You must be signed in to change notification settings - Fork 643
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
Support for Legends #276
Comments
Hi. Thank you very much for your interest in contributing code to mplfinance. The standard way to contribute code on GitHub is to use the "fork-clone workflow." I will assume you are somewhat familiar with The fork-clone workflow (click here) is essentially this:
Since this is your first time, I suggest the following:
You can contact me either by posting here on this issue, or by emailing me at [email protected] Please let me know if you have any questions. All the best. --Daniel P.S. there is also a little more detail about forking here. |
Thank you for the datailed steps, Daniel. I will try to follow this instructions and if any question comes up, I'll write you an email. One thing, do I need to run |
hth. --Daniel |
Okay. I think I did it right? Had a little bit of trouble at the beginning, but seems to be okay now. How can I know if I pushed all files right? Will I get any feedback or something? (push...pull...sometimes a litttle confused with that, but getting used to :) ) Let me know Daniel if its all okay! |
You can tell if the I can see your pull request here. It appears you did the push, and PR (pull request) correctly. One thing to be aware of (and this is good) ... the way GitHub PR's work is that as long as the PR is open, any additional pushes (on same branch) to your fork will automatically propagate into the PR. That's good, because if I ask for any changes all you have to do is make those changes and push again ... and those changes will propagate into the existing pull request. That said, it's late here and I have a very busy day tomorrow, so I may not have a chance to look over the code until Monday. Meanwhile I can tell you this much: The reason the Travis checks are failing on the PR is because one of the checks we put in place checks to make sure that each PR increases the version. You can do this by modifying one line in file version_info = (0, 12, 7, 'alpha', 1) to version_info = (0, 12, 7, 'alpha', 2) If you change the version and |
Please see code review comments that I have posted on the PR. |
Hi Daniel,
I just saw the comments you left on github.
I did not noticed that the labels needed to be treated as a tuple or list.
I agree with you that the labels should be converted to a list if the
label is only 1 string. I can add that change and also add the change of
using ' ' and not " " (sorry about that).
Regarding the part that you said that I removed something from the code,
when submitting the code I changed I noticed a lot of red lines that
supposedly I changed. However, I found it strange because I did not delete
any line and I guessed that those were deleted lines from before. The
process I did was:
1. Fork the repository
2. Clone the repository to my local machine
3. Replaced the cloned files with the files that I had changed, that where
in already in my local machine (those files were form the installation
files of mplfinance)
4. Commit
5. Push back to my repository
6. Made a pull request
Are these the steps that I should have followed?
How should we proceed?
Best regards,
PS
El lun., 19 oct. 2020 a las 14:41, Daniel Goldfarb (<
[email protected]>) escribió:
… Please see code review comments that I have posted on the PR.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#276 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOWW5JLDYVZ2BBGNZ3VJGQLSLR26HANCNFSM4SURPZKQ>
.
|
Pedro, The steps are correct except for step 3
It is a good thing that you were very clear about where the files came from, or I might still be very confused as to how this could have happened. It happens often, in many projects, that the version on pypi.org can differ from the version in the github repository. This is because one or more Pull Requests may be accepted into the repository before the maintainers decide to release a new version to pypi.org, thus the repository may be any number of commits ahead of the pypi.org installation version. In the case of mplfinance, presently pypi.org is on version 0.12.7a0, while the repository is at 0.12.7a1 and has the additional enahcnements to the
From the above we can see that the version you started with, the "base" version, from Aug 9 is Now we can generate the skipped version file, and the base version file, and do the three way merge as follows:
Now commit the merged contents:
Now test. If everything looks good, then I really enjoyed learning this today. I had personally never used git merge-file nor git cat-file so it was fun to learn and try. I hope that helps. Feel free to do whichever approach with which you are more comfortable. And I very much appreciate your willingness to contribute to mplfinance. I hope you are enjoying and learning from the process. All the best. --Daniel |
By the way, just in case you are not aware: There is a very easy way to install the version of any python package that you are working on. Simply go to the top level source code directory, the one that contains the file
Note the dot at the end. That is important. This creates an "editable" install ( |
Daniel,
Well I see now why the differences. I'm sorry to cause you such trouble, I
didn't knew that there could be different versions on github and pypi.
Actually, I thought that pypi always had the most recent version of the
code. That had some synchronism with github and automatically updates
repositories as they were updated in github. I'll pay more attention to
that in the future.
It's funny, because I know how to code reinforcement learning methods and
some other fancy machine learning stuff (especially for trading), but I
don't really know how to use git, which seems to be absolutely basic to
many people. I guess that's because I have always worked alone and haven't
had the need of using git, until now. So yeah, I enjoy the process of
learning new things. Especially if they are useful like git.
Regarding the solution to merge the files, I think that option 1 is
probably the fastest, but I would definitely like to try and learn how to
do option 2. So, if we are not in a rush I would like to give it a try and
learn how to use git properly. If not, then I can just edit manually the
code and use another git push.
Let me know what you think.
Best regards,
PS
El mar., 20 oct. 2020 a las 17:33, Daniel Goldfarb (<
[email protected]>) escribió:
… By the way, just in case you are not aware: There is a very easy way to
install the version of any python package that you are working on. Simply
go to the top level source code directory, the one that contains the file
setup.py and type the following:
pip install -e .
Note the *dot* at the end. That is important. This creates an "editable"
install (-e) which means that any changes you make to the repository
there will be immediately reflected in the installed package. No need to
reinstall every time you tweak some code. I find this very convenient when
I am working on a change.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#276 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOWW5JNKUAS3B2DYT7DXFX3SLXX25ANCNFSM4SURPZKQ>
.
|
Pedro, There is no rush. Take your time. Git, unfortunately, has a steep learning curve. I was using svn and resisted learning git for a long time. When I finally decided to learn git, I learned just a few basic commands and used only those commands for a long time until I felt more comfortable. It is becoming the "defacto" standard for collaboration, and for good reason. It takes a while to learn, but it is very powerful. I find when I'm wondering how to do something with git a little googling usually finds the answer. Regarding trying new things with git and the repository, as long as you do not push to your fork, if you make a mistake in your local repository you can always delete it (or move it aside) and clone again from your fork. I actually did that several times today (re-cloning your fork) as I was experimenting with differrent approaches to be able to merge the correct versions of the plotting.py file. All the best. --Daniel |
Hi Daniel,
I hope that this email finds you well. I did followed the steps of:
git cat-file -p 278e4b4:src/mplfinance/plotting.py > src/mplfinance/
plotting.base.py
git cat-file -p 7b1b722:src/mplfinance/plotting.py >
src/mplfinance/plotting.skipped.py
git merge-file src/mplfinance/plotting.py
src/mplfinance/plotting.base.py src/mplfinance/plotting.skipped.py
But I noticed that plotting.py now has like ~1800 lines. Maybe it's another
noob question of mine, but did I have to previously delete the plotting.py
file before merging? Because, if I'm not mistaken, a merge operation should
add the differences of plotting.base.py and plotting.skipped.py to
plotting.py, right?
And after all this, I should delete plotting.skipped.py and plotting.base.py
to only commit plotting.py?
I have a feeling that I did something wrong here :(
Best regards,
PS
El mar., 20 oct. 2020 a las 18:14, Daniel Goldfarb (<
[email protected]>) escribió:
… Pedro,
There is no rush. Take your time. Git, unfortunately, has a steep learning
curve. I was using svn and resisted learning git for a long time. When I
finally decided to learn git, I learned just a few basic commands and used
only those commands for a long time until I felt more comfortable. It is
becoming the "defacto" standard for collaboration, and for good reason. It
takes a while to learn, but it is very powerful. I find when I'm wondering
how to do something with git a little googling usually finds the answer.
Regarding trying new things with git and the repository, as long as you do
*not* push to your fork, if you make a mistake in your local repository
you can always delete it (or move it aside) and *clone again* from your
fork. I actually did that several times today (re-cloning your fork) as I
was experimenting with differrent approaches to be able to merge the
correct versions of the plotting.py file.
All the best. --Daniel
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#276 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOWW5JOE34LESXN2O2HVBQLSLX4UXANCNFSM4SURPZKQ>
.
|
Sounds like it might be OK. You're understanding seems to be correct. I'll take a closer look soon and let you know. |
This is correct!
Absolutlely not! At the beging of the above operations plotting.py should be the version that is currently in your fork, which is also identical to the version currently in the Pull Request. It contains 963 lines.
You can delete .skipped and .base if you want to. Certainly they should not be added to the repository. Yes, commit only plotting.py I just reproduced the process as follows:
You can see from the above that plotting.py went from 963 lines to 975 lines (by inserting the differences between .base and .skipped, as you mentioned above). The only thing I can think of that may have gone wrong is possibly you had either some other edits to the local src/mplfinance/plotting.py and/or some other commits. Did you try the At any rate, I reccommend that you go to a clean location (directory) on you system, and start with a fresh clone of your fork (as I did above). It should work then. Let me know. Thanks. |
Hi Daniel,
I'm sorry to write to you this late. I was quite busy this last two weeks,
but now I have some time to finish this git challenge :D.
I was reviewing the code I forked vs the code I merged and the code I
finally committed and I noticed that the issue could be this:
- Since I originally committed the files that I installed via pip, the
resulting commit was lacking the improvements done by TR.
- Then, I merged both files (cloning the repo in my local PC), but it
really got my attention that the resulting script was almost doubling the
amount of lines. In fact, the resulting script had twice the number of
functions (the ones that I modified plus the original ones).
- Then, I realized that probably the best way to do this is to delete the
first fork and perform a new one, delete the cloned repo from my local PC
and clone it again, do the changes in the code I am proposing and commit -
push them again (and finally do a pull request). Is this the way it should
be done?
Off-topic question: I wanted to ask you what kind of relation do you have
with trading? Are you an active trader or work for a trading institution?
I'm asking because I dream to become a professional algorithmic trader, but
sometimes I feel lost with the methods I'm taking. I have spent tons of
hours learning to code in python and using neural networks. I
actually got some promising results in the market with one of my first
reinforcement learning traders, but I feel I'm lacking some knowledge in
some points. Any advice?
Best regards,
PS
El jue., 22 oct. 2020 a las 23:38, Daniel Goldfarb (<
[email protected]>) escribió:
… ... a merge operation should add the differences of plotting.base.py and
plotting.skipped.py to plotting.py, right?
This is correct!
did I have to previously delete the plotting.py file before merging?
Absolutlely *not!* At the beging of the above operations plotting.py
should be the version that is currently in your fork, which is also
identical to the version currently in the Pull Request. It contains 963
lines.
And after all this, I should delete plotting.skipped.py and
plotting.base.py to only commit plotting.py?
You *can* delete .skipped and .base *if you want to*. Certainly they
should *not* be added to the repository. Yes, *only* commit plotting.py
------------------------------
I just reproduced the process as follows:
***@***.***:~/code/pr277$ git clone ***@***.***:ppseverin/mplfinance.git
Cloning into 'mplfinance'...
remote: Enumerating objects: 56, done.
remote: Counting objects: 100% (56/56), done.
remote: Compressing objects: 100% (33/33), done.
remote: Total 2525 (delta 27), reused 35 (delta 19), pack-reused 2469
Receiving objects: 100% (2525/2525), 45.94 MiB | 8.95 MiB/s, done.
Resolving deltas: 100% (1423/1423), done.
***@***.***:~/code/pr277$ cd mplfinance
/home/dino/code/pr277/mplfinance
***@***.***:~/code/pr277/mplfinance$ wc -l src/mplfinance/plotting.*
963 src/mplfinance/plotting.py
***@***.***:~/code/pr277/mplfinance$ git cat-file -p 278e4b4:src/mplfinance/plotting.py > src/mplfinance/plotting.base.py
***@***.***:~/code/pr277/mplfinance$ git cat-file -p 7b1b722:src/mplfinance/plotting.py > src/mplfinance/plotting.skipped.py
***@***.***:~/code/pr277/mplfinance$ wc -l src/mplfinance/plotting.*
953 src/mplfinance/plotting.base.py
963 src/mplfinance/plotting.py
965 src/mplfinance/plotting.skipped.py
2881 total
***@***.***:~/code/pr277/mplfinance$ git merge-file src/mplfinance/plotting.py src/mplfinance/plotting.base.py src/mplfinance/plotting.skipped.py
***@***.***:~/code/pr277/mplfinance$ wc -l src/mplfinance/plotting.*
953 src/mplfinance/plotting.base.py
975 src/mplfinance/plotting.py
965 src/mplfinance/plotting.skipped.py
2893 total
You can see from the above that plotting.py went from 963 lines to 975
lines (by inserting the differences between .base and .skipped, as you
mentioned above). The only thing I can think of that may have gone wrong is
possibly you had either some other edits to the local
src/mplfinance/plotting.py and/or so other commits. Did you try the git
log --pretty='%h %cd %an' -10 and did its output look like what I posted
above?
At any rate, I reccommend that you go to a clean location (directory) on
you system, and start with a fresh clone of your fork (as I did above). It
should work then. Let me know. Thanks.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#276 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOWW5JIYEUHKSJXNDZLEOMDSMDUAVANCNFSM4SURPZKQ>
.
|
Pedro- It is fine to do what you have outlined above. Sometimes when repo issues get complicated it is easier to just "start from scratch." I've do that from time to time. This is what I meant above when I suggested one approach would be to ...
So, yes, go ahead and do that. It should work. I'm not sure what GitHub will do about the existing PR. You can always click "Close pull request" at the bottom of the PR; (maybe even close it before deleting and recreating your fork). Regarding trading and career stories, I will send you an email and we can start an email thread specifically on that topic. No worries about being busy. Busy is good. Busy is a blessing. All the best. --Daniel |
Daniel,
The changes are ready. This time I'll wait for you before doing the pull
request.
Best regards,
PS
El mié., 4 nov. 2020 a las 20:39, Daniel Goldfarb (<[email protected]>)
escribió:
… Then, I realized that probably the best way to do this is to delete the
first fork and perform a new one, delete the cloned repo from my local PC
and clone it again, do the changes in the code I am proposing and commit -
push them again (and finally do a pull request). Is this the way it should
be done?
Pedro- It is fine to do what you have outlined above. Sometimes when repo
issues get complicated it is easier to just "start from scratch." I've do
that from time to time. This is what I meant above when I suggested one
approach would be to ...
re-apply your changes manually.
So, yes, go ahead and do that. It should work. I'm not sure what GitHub
will do about the existing PR. You can always click "Close pull request" at
the bottom of the PR; (maybe even close it before deleting and recreating
your fork).
Regarding trading and career stories, I will send you an email and we can
start an email thread specifically on that topic.
No worries about being busy. Busy is good. Busy is a blessing. All the
best. --Daniel
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#276 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOWW5JOMCXT4P6PDG6FB77LSOHQ2BANCNFSM4SURPZKQ>
.
|
OK. Thanks. I'll take a look tomorrow. |
My apologies for not updating this issue sooner. I did some testing of the code, and experimented with a lot of various use cases. It turns out this is not a simple as we thought. While it is relatively easy to get legens/legend-labels working well for some use-cases, it turnsout that (as implemented) they would not work in many other use cases. This is because legends are dependent on having access to each matplotlib Artist object that you want to label. Unfortunately, the way mplfinance is currently structured, some of the Artist objects are readily available while other are not. This is not to say that we should not do this. On the contrary, based on my experiements, I have some ideas on how to revamp the code to make legends easier, but the revamping may take some time. That is why I decided to first focus on some easier enhancements. (You might think at first that legends would be easy, but alas.) I expect a few more weeks of working on other enhancement requests, and then I plan to come back to this. |
Any updates on this? |
See this example. I didn't find anything in the official docs, but this comment helped immensely! |
Regarding this question I posted 2 days ago, I started to inspect the code and made some changes. Now I'm able to do this:
mpf.make_addplot(bars[["sma_10","sma_200"]],labels=["sma_10","sma_200"])
Is this useful to the community? What other testing is needed and how do I submit the changes to see if they are approved?
Its the first time I change the code of a library I downloaded.
Hope you will find it useful. The results are in the image below.
The text was updated successfully, but these errors were encountered: