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

Fix compatability with latest plotly and chart_studio #203

Closed
wants to merge 13 commits into from
Closed

Fix compatability with latest plotly and chart_studio #203

wants to merge 13 commits into from

Conversation

nateGeorge
Copy link
Contributor

@jorgesantostr I'm happy to help create the new release if you can give me write access to the github repo (i.e. add me as a collaborator), and write access to the PyPi package (i.e. add me as maintainer or owner of the PyPi package).

A number of fixes were required to make cufflinks compatabile with the latest version of plotly, and the online chart_studio library (also from plotly).

This seems to pass the nosetests, and I reran the jupyter notebooks to make sure they work. There is a weird FutureWarning from pandas in there, which seems to be related to numeric conversions. I couldn't track down where this is coming from, but see the .ptp FutureWarning in the notebooks for more. Seems to come from nanptp in Pandas.

If this doesn't get merged (seems like activity has stalled on this repo this year), you can install this version with:

git clone https://github.com/nateGeorge/cufflinks.git
cd cufflinks
python setup.py install

Related to PR #202 , #195, and #198, as well as issues #196, #194, and probably more. Thanks to @eholic for the subplots fix.

@nicolaskruchten
Copy link
Collaborator

Hi @nateGeorge ! How would you describe the differences between your changes here and those in #202 which seem to be basically the same except with less whitespace churn?

@eholic
Copy link
Contributor

eholic commented Oct 23, 2019

Hi @nicolaskruchten . I used chart_studio.plotly in #202, but @nateGeorge try to avoid it for offline features. I guess that's the main difference.

@santosjorge
Copy link
Owner

@nicolaskruchten you tell me which one to go for and I'll get it done straight after. Thank you both.

@nateGeorge
Copy link
Contributor Author

@nicolaskruchten Yes, there is the unfortunate whitespace thing in the second commit, must be vscode or atom auto-removing extraneous whitespace. Some of the differences between #202 are:

  • I reran jupyter notebooks to ensure things were working
  • Fixed the FigureFactory import for OHLC plots
  • handled the case where figure.layout['title']['text'] is None in plotlytools.py
  • used 'brown' instead of java hex code for color fix
  • I had commented out from chart_studio.plotly import plot in init, I think this was due to offline vs online. I don't remember the issue I was running into here a few weeks ago, but maybe it would be ok to keep the from chart_studio.plotly import plot line in init?

#202 also has a statsmodels addition in the requirements file that I don't have.

Why not just merge both of them? If the whitespace thing is a problem, I can probably re-do my commits and create another PR.

@nicolaskruchten
Copy link
Collaborator

Ok thanks for the response! I think I’ll review #202 and we’ll get it merged in and then you can PR again on top of that if desired with any extra deltas that are important?

@nicolaskruchten
Copy link
Collaborator

OK, I've merged #202 and a couple of the fixes from this PR. Please feel free to submit a new PR on top of master with anything I missed!

@nateGeorge
Copy link
Contributor Author

Great, thanks! Looks good except a recent commit broke the bestfit function for time series. The fix is small and I'm issuing a PR shortly.

@nateGeorge nateGeorge closed this Oct 23, 2019
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.

None yet

4 participants