nateGeorge commented 5 years ago

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

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

Hi @nicolaskruchten . I used chart_studio.plotly in https://github.com/santosjorge/cufflinks/pull/202, but @nateGeorge try to avoid it for offline features. I guess that's the main difference.

santosjorge commented 5 years ago

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

nateGeorge commented 5 years ago

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

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

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

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.

santosjorge / cufflinks

Fix compatability with latest plotly and chart_studio #203

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