search

RJT1990 / pyflux

Open source time series library for Python
BSD 3-Clause "New" or "Revised" License
2.1k stars 240 forks source link

Allow plot axis specification #81

Closed bgbg closed 6 years ago

bgbg commented 7 years ago

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 commented 7 years ago

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

RJT1990 commented 7 years ago

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!

bgbg commented 7 years ago

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 commented 7 years ago

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.