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

Added xlabel= kwarg to mpf.plot() #564

Merged
merged 6 commits into from
Oct 31, 2022

Conversation

vedant-gawande
Copy link
Contributor

Please merge this repo as you can see i have done the work as per requirement

@vedant-gawande vedant-gawande changed the title Added xlabel= kwarg to mpf.plot() #427 Added xlabel= kwarg to mpf.plot() Oct 21, 2022
@DanielGoldfarb
Copy link
Collaborator

@vedant-gawande
Ved,
Thanks so much for contributing. At a glance it looks good. I'm short on time for this project today, but will look into merging this early next week. All the best. --Daniel

@DanielGoldfarb
Copy link
Collaborator

issue #427

@vedant-gawande
Copy link
Contributor Author

@vedant-gawande Ved, Thanks so much for contributing. At a glance it looks good. I'm short on time for this project today, but will look into merging this early next week. All the best. --Daniel

Yeah sure sir,
No problem

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.

see comments. please make minor change requested and push to your fork (will automatically propagate to the PR).

@DanielGoldfarb DanielGoldfarb dismissed their stale review October 31, 2022 20:56

different changes were needed and were made

@DanielGoldfarb
Copy link
Collaborator

It turns out that apparently this was not tested at all. A little bit of testing showed that it was not working for all cases where the main plot is not the bottom-most panel.

  1. In panels mode, the xlabel should always be on the bottom panel, and it makes no sense for addplot to support xlabel (since it will always be on the bottom-most panel). Therefore moved xlabel implementation into _set_ticks_on_bottom_panel_only() when in panels mode.
  2. In external axes mode, the user can always call Axes.set_xlabel() directly on whichever axes they choose, so again no need to support xlabel in addplot.

This changes were made and merged into the PR.

@vedant-gawande Thank you for contributing to mplfinance! Wishing you all the best.

@DanielGoldfarb DanielGoldfarb merged commit 8d46383 into matplotlib:master Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants