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

Allow plot axis specification #81

Closed
wants to merge 2 commits into from
Closed

Allow plot axis specification #81

wants to merge 2 commits into from

Conversation

bgbg
Copy link

@bgbg bgbg commented Jan 30, 2017

Addressing #73: every plot_* function now accepts an optional axis
keyword argument. If supplied, the function will draw the graphs on
that axis. The user may later use this axis.

Addressing  #73: every `plot_*` function now accepts an optional axis
keyword argument. If supplied, the function will draw the graphs on
that axis. The user may later use this axis.
@RJT1990
Copy link
Owner

RJT1990 commented Jan 30, 2017

Thanks for the PR! Will review tomorrow and get back to you

@RJT1990
Copy link
Owner

RJT1990 commented Jan 31, 2017

Hey Boris - looks great. Only thing is, can I ask if there was a reason you removed the imports from the methods? I think we need them in the plot methods so they are an optional dependency, otherwise it can lead to problems with Tkinter (See #51) and also Jenkins builds. Once this is clarified/fixed, then happy to merge!

@RJT1990 RJT1990 self-requested a review January 31, 2017 16:46
`ax.title` is a string. To set an axis title, one need to call the
`ax.set_title` function.
@bgbg
Copy link
Author

bgbg commented Feb 2, 2017

First of all, I fixed a bug related to setting axes title.
As to the question about moving the imports. I thought that would be a better program structure. If you want to provide a mechanism for handling optional imports, I'd suggest adopting this approach.

Also note that I didn't test the code, as I can't build it on my computer: it keeps failing during the cythonization phase and I don't have the time to debug it.

Let me know if you want me to implement the optional imports in the way of creating a dedicated class (like in the link I gave above) or by moving the imports into the individual functions.

@RJT1990
Copy link
Owner

RJT1990 commented Feb 2, 2017

That approach makes sense - agree it's more targeted than simply importing within each plotting method. Also, I've uploaded the tools folder for the repo which should allow you to cythonize your build locally - let me know if that works for you.

@bgbg bgbg closed this Jan 28, 2018
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