Skip to content

[DO NOT MERGE] slider-generating transform #1099

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

Closed
wants to merge 13 commits into from

Conversation

rreusser
Copy link
Contributor

This is a rough cut at a transform that generates a slider based on filter transform input.

@rreusser
Copy link
Contributor Author

rreusser commented Oct 31, 2016

@etpinard I can't quite figure out the correct sequence. At the moment, you have to double click to relayout the plot before the slider will show up. I think I need to similarly readjust the sequence of events and pull a couple tricks in order to get defaults supplied and the slider options filled in correctly.

I'll update this with my analysis as I work through it:

  • I'm doing everything in the calcTransform step at the moment… trying to reconstruct the reason, but the problem is that supplyDefaults doesn't clean up the definition I add if I do it this late.
  • Issue: even if I run the transform in .transform, supplyLayoutGlobalDefaults has already been run when supplyDataDefaults is executed (edit: maybe it's supplyLayoutModuleDefaults` that I want double-edit: except this should have nothing to do with the fact that it's cartesian 😞) so that I probably need to supplyDefaults on the slider manually, which seems to step uncomfortably outside of the normal sequence of events.

Also: It seems like it will be very difficult to get this not to fail in the case where two traces talk to the same slider and have different target data or even the same target data in a different order. The behavior seems probably just undefined in this case.

@rreusser
Copy link
Contributor Author

rreusser commented Oct 31, 2016

Clearest case that is weirdly ambiguous:

  • two traces that animate together. Each trace needs this slider-populating transform. That means each trace needs target data. The target data determines the content and order of the slider steps. What if the target data doesn't match? What if some are missing in one or the other? What if the steps are the same but in a different order?

@etpinard
Copy link
Contributor

I'm doing everything in the calcTransform step at the moment

Have you tried putting everything in a (regular supply-defaults) transform instead? That should fix everything albeit would make things a little slower.

@rreusser
Copy link
Contributor Author

rreusser commented Oct 31, 2016

@etpinard I split half (slider setup) into .transform and left the other half (frame setup) in .calcTransform. It seems to work now… https://rreusser.github.io/animation-experiments/#gapminder-filtered

@rreusser
Copy link
Contributor Author

rreusser commented Oct 31, 2016

@etpinard @chriddyp Here's a first cut a working slider-generating transform:

The input: https://github.com/rreusser/animation-experiments/blob/master/src/plots/gapminder-simplified.js
The output: https://rreusser.github.io/animation-experiments/#gapminder-simplified

Features to notice:

  • The first trace is missing 1952. The second is missing 1962. The slider combines years from both.
  • China is missing for 1967. Same as above, things appear and disappear as you'd expect 🎉
  • The slider can already exist, but it doesn't have to. If it doesn't exist, it'll just get the defaults.
  • The animation options must get passed twice, but get merged and only used once :/
  • Animation only works with one slider at a time. It would need to scope the bindings more carefully in order to work with two.

@chriddyp
Copy link
Member

chriddyp commented Nov 1, 2016

ambiguous [...] two traces that animate together

From a data point-of-view, I think that non-ambiguous way this would be represented would be with the group-by transform, i.e. "one trace" with

x: lifeExp,
y: gdp
size: pop
animate_by: year
group_by: continent

@chriddyp
Copy link
Member

chriddyp commented Nov 1, 2016

Looks like you've figured out the ambiguity case though (by taking the union of the target data?). Looks really good! 🎉

@rreusser
Copy link
Contributor Author

rreusser commented Nov 1, 2016

@chriddyp The specific problem is that accomplishing this as data transforms leads to a need to specify layout-type config on multiple data traces. Short of building a concept of layout transforms that I think is best avoided for now, I don't see any way around that. As long as the resulting syntax stays straightforward, the current form is really just about the best I can do. I'm worried that shorthand notation like animate_by or group_by would add more coupling between the parts in a way that locks in the features for specific use-cases.

I'm working on tests now, but if this works for you, the steps the front end will need to accomplish will be:

  1. add ids and text columns containing strings that identify each point
  2. add a populate-slider (needs better name) transform. The defaults are probably fine, so it may not even need much config.
  3. add a filter transform containing per-point target data to be grouped into slider steps
  4. (optionally) add a slider that will receive the steps. (It creates this automatically if not specified, but it's better if it exists so that the layout can be modified.)

To elaborate slightly, I totally get the desire to boil this down to a couple keywords, but I think we're a long way away having the features, the implementation, and lots of other details locked down well enough to take that approach. For example: it's not clear to me how we'd pass animation config or slider config with a single keyword, and it seems like a very bad idea to add all of the necessary keywords to the trace itself.

@jackparmer
Copy link
Contributor

Hey @rreusser - Do you have an update on this one?

@rreusser
Copy link
Contributor Author

rreusser commented Nov 8, 2016

@jackparmer I had a long agonizing talk with @etpinard yesterday about the conclusion that there's no way to make this feature reasonably robust with transforms as they currently exist. Transforms aren't written in a way that can handle layout components in any reasonable manner. He pushed this branch which adds some additional capability to transforms. I'm rewriting now in a way that should remove 80% of the complexity. I'm not 100% convinced it'll be trivial now, but it should be at least an order of magnitude (or two?) easier. Will update with progress ASAP, but I expect that after two full rewrites already, a day should be adequate here.

@rreusser rreusser changed the title [DO NOT MERGE] Rough cut at slider-generating transform [DO NOT MERGE] slider-generating transform Nov 8, 2016
@rreusser
Copy link
Contributor Author

rreusser commented Nov 8, 2016

@etpinard I've completely rewritten this transform based on yesterday's discussion. Once it was implemented correctly, all of the existing tests passed with only a small naming change! 🎉

One thing I couldn't get working: when all transforms are deleted, the module is no longer in the and so the transform doesn't get run to clean up after itself. This is probably a secondary concern, but other than that, I feel not so bad about this. Again, it's an order of magnitude easier than what I was doing before.

cc @chriddyp

@rreusser
Copy link
Contributor Author

rreusser commented Nov 8, 2016

Oh, and I should mention: I implemented it in this repo since it was much easier to develop here. Streambed is up and running for me though, so should be easy to pick this up and drop it in that repo. I do need to separate out the additional supplyLayoutDefaults step for transforms though. I'll create a separate PR for that.

@rreusser
Copy link
Contributor Author

rreusser commented Nov 8, 2016

Next steps for finishing this off:

  • transform -> supplyLayoutDefaults PR
  • ignore null frames PR
  • add a couple more tests to the transform
  • move transform code to streambed

See example at (play button needs naming fix in order to work):
https://rreusser.github.io/animation-experiments/#gapminder-simplified
https://github.com/rreusser/animation-experiments/blob/master/src/plots/gapminder-simplified.js

@etpinard
Copy link
Contributor

etpinard commented Nov 8, 2016

@rreusser

I can take care of

  • transform -> supplyLayoutDefaults PR

if you want.

@rreusser
Copy link
Contributor Author

rreusser commented Nov 8, 2016

@etpinard That's okay with me since you know obscure concerns better than I do. If so though, note the extra changes required to pass transitionData (which seemed like a reasonable thing to pass across the board): https://github.com/plotly/plotly.js/pull/1099/files#diff-ad4f76ccd6044ed16514297078e13b84R1036

@rreusser
Copy link
Contributor Author

rreusser commented Nov 8, 2016

@etpinard Current state:

I'm happy with how this is working. There are a couple prerequisite PRs still making their way through. Moving this code to streambed is easy. Blocking: the version of static plotly.js in streambed. Is updating straightforward?

@etpinard
Copy link
Contributor

etpinard commented Nov 9, 2016

transferred to https://github.com/plotly/streambed/pull/8428

@etpinard etpinard closed this Nov 9, 2016
@etpinard etpinard deleted the magic-slider-generating-transform branch November 9, 2016 18:42
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.

4 participants