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

Enable interactive visualizations #55

Merged
merged 66 commits into from Jul 27, 2019

Conversation

@saulshanabrook
Copy link
Member

commented Apr 8, 2019

Fixes #54

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2019

Ooh, marking as draft, that's new to me.

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

One question I have about this: how expensive is the actual vega render? You have been going down this road to get signals and pulses working with vega, and that may end up being the right solution. But how competitive would it be to do something like:

  1. Produce an ibis expression with an ipywidget observable.
  2. Evaluate the expression, make an Altair plot with that data.
  3. Make an ipywidget slider (or vdom slider).
  4. On a change of the slider, update the ibis expression and evaluate it.
  5. Produce a new altair visualization with the new data, send it to the old display id.

That is to say, we can have vega make slider, decide when to wire the transform machinery up to this, and when to call out to ibis. Or, we can do it all in Python and ipywidgets. The cost would be rerendering the whole vega scene, but the benefit would be less mucking about in vega internals. If the rerendering of vega is cheap compared to everything else (not sure whether that is true), the difference might be negligible.

I'm sure you've already thought through a lot of this @saulshanabrook, so I'm largely just thinking out loud here.

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Thanks for the thoughts on this. Yeah, that a valid way of doing it. We are actually doing this now, we have a demo setup that works like that in https://github.com/Quansight/jupyterlab-omnisci/blob/master/notebooks/Ibis%20%2B%20Altair%20%2B%20Extraction.ipynb

This works OK for a slider, but won't cover use cases like cross filtering or zooming, where the interactions are very closely tied to the existing visualizations.

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

Yeah, you're right, that won't allow us to do some of the fancier stuff.

My current thinking is that we should keep the OmniSci session on the server. We already have an authenticated session there, and as long as we are doing the roundtrip to Ibis, we may as well evaluate there as well. It means somewhat more network traffic, as the data is not going directly to the frontend, but I'd really like to avoid more places where we are storing credentials frontend.

So in my mind, this would look somewhat different from the "Vega at scale" demo, since the one doing the talking to the server would be pymapd. A side effect of this is that it would make it easier to work with other database backends as well...

What do you think @saulshanabrook?

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

In a similar vein, I've been thinking of rewriting the SQL editor mimerenderer to pass its queries through the kernel so that credentials are not hidden in notebook mime-outputs.

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

@ian-r-rose Yep! This is all in line with what I had been thinking too, for similar reasons (not having to copy credentials, applicability to other backends).

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

In a similar vein, I've been thinking of rewriting the SQL editor mimerenderer to pass its queries through the kernel so that credentials are not hidden in notebook mime-outputs.

Ah interesting, yeah I think that makes sense!

Putting potentially sensitive things in the outputs by default seems like a bad idea.

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

Ayup.

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

@saulshanabrook As I'm working through trying to understand the data flow here, I'm starting to rebase and update this PR to try to get it to compile. Do you mind if I push changes here?

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

@ian-r-rose Go for it!

It doesn't compile at the moment.

If you get stuck, I am happy to help get it to a usable place, I am not sure what state I left it in.

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

@ian-r-rose Have you done any work on this branch? I want to pick it up again today.

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Apr 22, 2019

Yes, I've done a bit, but not gotten anything really working, just trying to get it to compile and use some typescript. Want me to push it here? I was also planning to work on it today.

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

@ian-r-rose Sure!

I would be down to try some peer coding, if you are game. Like you could screen share and we could try to work through it together.

@ian-r-rose ian-r-rose force-pushed the vega-lite-extract branch from 8fe6689 to 9449abe Apr 22, 2019

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Apr 22, 2019

@saulshanabrook That would be great. Any particular time work well for you? I should be fairly flexible starting in about an hour.

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

I sent you an invite for this afternoon.

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

Yep.

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

And publish a version 1.0.2 that reverts back to the 1.0.0 source.

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

Yes.

@saulshanabrook

This comment has been minimized.

Copy link
Member

commented on 57844fe Jul 19, 2019

Nice that looks better! Thanks.

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

Still trying to figure out how to avoid comm errors if the notebook tries to render with a nonexistent expression (as happens if you open a notebook with a previously-run spec)

@ian-r-rose
Copy link
Collaborator

left a comment

Okay, at this point I am fairly happy with how this looks. If you take another look through and are happy as well, I say we move forwards with this, @saulshanabrook!

I would like to suppress that altair warning about being unable to infer a dtype, but have not been successful in that so far. But that's a small thing.

return name.startswith(DATA_NAME_PREFIX)


def _retrieve_expr_key(name: str) -> typing.Optional[str]:

This comment has been minimized.

Copy link
@ian-r-rose

ian-r-rose Jul 19, 2019

Collaborator

At one point I very briefly grokked why this function exists, and why the name of the expression in the queryibis transform is different from the name of the data source. That is, one is named ibis:{hash}, and the other is named {hash}. Now I cannot remember why that is the case. Can you refresh my memory, and maybe add a docstring here @saulshanabrook ?

This comment has been minimized.

Copy link
@saulshanabrook

saulshanabrook Jul 22, 2019

Author Member

Will do. I think I will add a big docstring to this file to describe the "flow" with the different parts, referencing functions as needed, if that sounds OK.

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

I've attempted to suppress the warning with another monkey patch. I'm not sure if it's a good idea, however, so open to backing that out again.

@gnestor

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

Do I need to make any changes to vega4-extension in jupyter-renderers?

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Jul 22, 2019

I have a PR open removing it entirely, at least while it is being published from jupyterlab/jupyterlab/packages

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

Do I need to make any changes to vega4-extension in jupyter-renderers?

Otherwise, this should operate independently and does not depend on any other vega extension

saulshanabrook added some commits Jul 25, 2019

@ian-r-rose

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

I'm happy with this when you are @saulshanabrook!

@saulshanabrook saulshanabrook merged commit 3bd4c64 into master Jul 27, 2019

Interactive Visualizations automation moved this from In progress to Done Jul 27, 2019

@saulshanabrook saulshanabrook deleted the vega-lite-extract branch Jul 27, 2019

@domoritz

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

Ohh. Is there an example I can play with somewhere?

@saulshanabrook

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2019

@domoritz yeah try the binder link in the readme, there is an interactive example in the notebook that opens up. Also you can try this notebook in the same binder, it's more of a WIP but it has some crowsfiltering working https://github.com/Quansight/jupyterlab-omnisci/blob/master/notebooks/Charting%20Example.ipynb

@domoritz

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.