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

Adding animation opts to python interface #1503

Merged
merged 8 commits into from
Apr 10, 2019
Merged

Adding animation opts to python interface #1503

merged 8 commits into from
Apr 10, 2019

Conversation

TakodaS
Copy link
Contributor

@TakodaS TakodaS commented Apr 9, 2019

#1496 Added kwarg in plotly/offline/offline.py to allow animation options to be passed through the python interface to the plotly.js functions.

@jonmmease
Copy link
Contributor

Hi @TakodaS, thanks for the PR.

I think I'll manually merge your changes into #1474 since that PR does a big refactor to the HTML handling code and so this would cause some merge conflicts.

In thinking about the API a little more, here are two thoughts.

  1. I'd like for users to specify the animation options as a Python dict rather than a string containing JSON. This is more consistent with how the rest of the plotly.py APIs behave.
  2. We recently added the auto_play argument to ploty.offline.iplot and plotly.offline.plot to control whether the animation is started automatically. I'm thinking about overloading this argument to handle the animation options rather than introduce a new argument that would have to be consistent with auto_play. Here's the behavior I'm picturing:
  • False, don't autostart the animation
  • True (the default), automatically start the animation with default properties
  • a dict (e.g. {'frame': {'duration': 1}}), automatically start the animation with the specified properties.

How does that sound?

@TakodaS
Copy link
Contributor Author

TakodaS commented Apr 9, 2019

Hi @jonmmease,

  1. Absolutely.

  2. I am against this. animation_opts syntactically makes the python API similar to the R plotly API. I would suggest keeping the two separate or in fact including auto_play as an option in animation_opts.

@jonmmease
Copy link
Contributor

I am against this. animation_opts syntactically makes the python API similar to the R plotly API

Here's my reasoning. In the R API, I believe the animation_opts argument is applied when you press the play button (do animations also auto play with the R API?). But this will not be the case for the animation options passed to plot/iplot as these options are only applied when calling the initial Plotly.animate call to kick off the initial animation sequence. The animation options applied when pressing a button are controlled by the button arguments in the figure specification. So they are more like auto_play_animation_opts, because if a user sets auto_play to False, then any animation options passed to plot/iplot won't be used for anything. I was thinking that this fact might be clearer in the API if the options were included as auto_play options. I would like to default the auto_play argument to False in version 4, so I'm concerned that it will be confusing that passing animation_opts has no effect unless auto_play is also set to True.

Separating animation options from auto_play does make sense for plotly_express though, because plotly_express, like the R API, constructs the slider/animation button internally and so it could make sure to use the same options for button press as it would use for auto_play.

What do you think @nicolaskruchten?

@nicolaskruchten
Copy link
Contributor

I think Jon's reasoning makes sense that animation_opts won't do anything outside of auto_play mode, and in the short run we can't remove the auto_play kwarg to move it into animation_opts without breaking backwards compatibility for this newly-introduced kwarg. I'd be fine with leaving the new kwarg on its own given that auto_play currently defaults to True and then revisiting this later: either rolling auto_play into animation_opts or vice versa (I have no super-strong preference there).

@jonmmease
Copy link
Contributor

I updated this PR to use the new renderers framework introduced in #1474. I kept the separate animation_opts argument and updated it to expect a dict instance.

@jonmmease jonmmease added this to the v3.8.0 milestone Apr 10, 2019
@jonmmease
Copy link
Contributor

Merging

@jonmmease jonmmease merged commit 82a62ff into plotly:master Apr 10, 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.

3 participants