-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 iplot offline #599
Fix iplot offline #599
Conversation
Tests passed locally but getting strange encoding error on circle. It looks like its in cc: @choldgraf |
The last bit of the error message:
|
Hmm. I think the tests need some attention in general. I'll come back to this later in the day and try to fix up our test suite. That is an old At any rate, I'll take a look later today. |
Changes look good! I'll try to get a patch in to fix the test suite and we can get this in soon! |
hey all - shall I take a look at this now to see if it works? |
Go ahead! I just rebuilt the test and it failed again. |
ok, will try to spend some time on this over the weekendOn Wed, Nov 2, 2016 at 10:26 AM Adam Kulidjian notifications@github.com
|
I can confirm that iplot now works on this branch. Still slow as heck but it does plot :) |
Do you know which PR solved it? I was working on one but the tests were not passing for some reason... |
I just pulled the code on this PR and tried it out |
Oh wait, I'm ridiculous. This PR was the PR I was talking about. Yeah, it works but the tests are still failing for some reason... 😕 |
@choldgraf @theengineear Working on this now. Finally getting test failures to occur locally - - helpful! |
Getting there. Cleared up this error which #614 was getting: |
@chriddyp Can I get a 💃 ? Finally we have success. Who else should I ping in the future? Is one reliable person enough? 😄 |
Me or @theengineear ! |
K, sounds good. Let me know if you approve here. |
@@ -869,7 +868,7 @@ def _fill_in_response_column_ids(cls, request_columns, | |||
for req_col in request_columns: | |||
for resp_col in response_columns: | |||
if resp_col['name'] == req_col.name: | |||
req_col.id = '{0}/{1}'.format(grid_id, resp_col['uid']) | |||
req_col.id = '{0}:{1}'.format(grid_id, resp_col['uid']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So did making plots based off of grid columns just not work before? Can you show me an example where this didn't work before but now, with this fix, it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it wasn't working until I made the switch. Here's one example that didn't work before the change that worked after:
c1 = Column([6, 6, 6, 5], 'pillow')
c2 = Column(['a', 'b', 'c', 'd'], 'couch2')
g = Grid([c1, c2])
py.grid_ops.upload(g, 'pillow_and_couch_grid222', auto_open=False)
url = py.plot([Scatter(xsrc=g[0], ysrc=g[1])], auto_open=False, filename='222pillow222')
I'm curious about something though...when plotly makes the request to V2 for the upload of the grid, prior to that, the local grid has blank id
s (eg. grid[0].id
returns ""
) and after the request, your local grid has the proper filled in id eg. "AdamKulidjian:1782:u3fg1"
How is my local grid getting updated if only a url after py.upload()
is run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is my local grid getting updated if only a url after py.upload() is run?
py.grid_ops.upload(g)
updates the columns in g
. Those columns in g
have the same reference as c1
and c2
, so changing them in g
changes c1
and c2` too. See http://nedbatchelder.com/text/names.html for some more info.
The ids
themselves are assigned by the plotly server.
@@ -6208,6 +6207,9 @@ def create_dendrogram(X, orientation="bottom", labels=None, | |||
if len(s) != 2: | |||
exceptions.PlotlyError("X should be 2-dimensional array.") | |||
|
|||
if distfun is None: | |||
distfun = scs.distance.pdist | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the motivation behind this change? It seems like by removing it from the call signature it is less informative to users about what distfun
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, but for some strange reason the tests were failing here when scs()
functions were in the calls. By putting them in the body of the functions it cleared up the problem.
I thought it had something to do with the order of the
try:
import scipy as scp
_scipy_imported = True
except ImportError:
_scipy_imported = False
or such but it wasn't related
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. should scs
by scp
in this case? Also, what happens if a user runs this function but they don't have scipy
installed? It's not a required dependency of the plotly
package (and it shouldn't be) but folks might be surprised if running the function fails due to a missing module.
What do we do in other parts of the code that use scipy
? Do we just let it fail or do we tell users that they need to install scipy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. I feel like the error messages may do that enough. I mean, scipy
is pretty commonplace as well. For this commit at least I don't think we should add the error messages, as that may be a big project in all parts of plotly.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. It's just that the error won't be "scipy is not defined" because it won't break on the import scipy
step, it'll break on the scs.distance.pdist
step which means that the error will probably be scs is not defined
which isn't very intuitive or helpful.
Could you try creating a clean virtualenv without scipy and try calling this function and seeing what the error message is? Scipy may be common but Plotly may be the first package and experience that users have in Python and we want to make it as easy as possible for people to get going. We can fix the issue in a separate PR but it'd be nice to know how this will break for our users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for looking into that!
Just left a couple of questions that I'd like to understand better before we merge. Otherwise looks good! |
That's what I suspected, but wasn't prepared to dig into streambed ;) |
OK, this looks good to me. 💃 |
Re iplot issue brought up in #593 (comment)
Fixed by just removing the unnecessary
show_link
andlink_text
from the_plot_html()
call in this line.plot()
looks fine as it hasshow_link
andlink_text
in the signature.