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

Improvements to QuickPlot #871

Closed
valentinsulzer opened this issue Mar 4, 2020 · 11 comments · Fixed by #886
Closed

Improvements to QuickPlot #871

valentinsulzer opened this issue Mar 4, 2020 · 11 comments · Fixed by #886
Assignees

Comments

@valentinsulzer
Copy link
Member

valentinsulzer commented Mar 4, 2020

QuickPlot could do with a few updates. Feel free to add more to this list:

  • More obvious error when two variables have different dimensions
  • More obvious error when axlims are inf
  • Allow plotting a single 3D variable (contour plot)
  • Make the x-axis label "x" when the spatial scale isn't found (or raise error?)
  • Should there be default output variables? Or should they always have to be passed in (which then makes it more obvious how to change them?)
  • Try to clean up set_output_variables to make it more obvious what is being done
  • Slider sometimes obscures the figures
  • Should we be calling it SliderPlot?
@Scottmar93
Copy link
Contributor

My thoughts are:

  1. Agree
  2. Agree
  3. Yes we really need this.
  4. If spatial scale not found then raise error
  5. Yes defaults are good for pybamm.QuickPlot
  6. Okay
  7. Okay
  8. Sure. Maybe just make quickplot a method of simulation that uses the default variables.

Some additional ideas that are non essential but might be nice:

  • Plot all variables in dimensional form?
  • when plotting multiple models, use dashes and dots for the lines
  • if passing in multiple models, allow labels for each model to also be passed in
  • allow linestyles/colors to be passed for each model
  • add a figsize argument

Another thought though this really is just extra. It might be nice to have helper functions in simulation:

  • slider plot (for what you describe above)
  • quickplot (slider with defaults)
  • Plot (just accepts 2 variables and plots them against each other)
  • Plot2D (takes in 3 variables and produces the contour plot)

@rtimms
Copy link
Contributor

rtimms commented Mar 5, 2020

My thoughts:

  • I'm also for having default variables, as it is nice just to get a quick overview without worrying about the details.
  • I think the class should still be QuickPlot, as you can just call plot to get a static plot at a fixed time. happy for dynamic_plot to be renamed slider_plot. Maybe there could be some default collections for plots people use a lot, but this doesn't necessarily need to be done in this issue and may be best for users to implement themselves
  • For the default li-ion some of the variables and dimensional and some aren't, so it would be to make this consistent
  • It looks like labels, linestyles and colors are in and can be user supplied. the linestyles aren't looped over as it looks like there is a typo in line 339: linestyles[j] should be linestyles[i]. adding a figsize would be nice
  • maybe by default the time should be in seconds (or in seconds if the simulation is less than e.g. 2hrs). I think it is easier to read off x seconds rather than 0.x hours. maybe you could even pass how you want time to be represented e.g. seconds, hours, discharge capacity
  • it would be nice to have the option to pass an array of times to plot to get a single figure with profiles of the variables at different time

@valentinsulzer
Copy link
Member Author

All sound like good points. re: linestyles, the current functionality is that you can plot several variables on the same plot and they will be differentiated by the linestyles, which is why the different solutions are only differentiated by colour.
For example

pybamm.QuickPlot(
    solution,
    output_variables=[
        "Negative particle surface concentration",
        "Positive particle surface concentration",
        ["Electrolyte current density", "Electrode current density"],
        "Terminal voltage [V]",
    ],
)

gives
example

@rtimms
Copy link
Contributor

rtimms commented Mar 5, 2020

ah nice! I didn't know you could nest lists in that way to use in QuickPlot

@Scottmar93
Copy link
Contributor

When we call ipywidgets within simulation for slider plots in jupyter notebook, I think we should also pass the argument continuous_update=False as this seems to give more responsive behaviour :)

@valentinsulzer valentinsulzer self-assigned this Mar 10, 2020
@valentinsulzer
Copy link
Member Author

Are you thinking to pass this in from the notebook or detect that it is being run from a notebook?

valentinsulzer added a commit that referenced this issue Mar 11, 2020
@Scottmar93
Copy link
Contributor

Was thinking to just change line 536 of simulation to:

widgets.interact(
                plot.plot,
                t=widgets.FloatSlider(min=0, max=plot.max_t, step=0.05, value=0),
                continuous_update=False
            )

@Scottmar93
Copy link
Contributor

Regarding linestyles, that is really neat and I didn't know you could do that either. I still think it would be nice to do models by linestyle though (if multiple variables are not plotted on the same figure) since I would use this more frequently.

@rtimms
Copy link
Contributor

rtimms commented Mar 11, 2020

can we just move isnotebook to QuickPlot so we can use it directly in notebooks without having to do the slider widget manually?

@Scottmar93
Copy link
Contributor

One additional thing that would be nice, is if QuickPlot can accept solution objects and simulation objects. If a simulation object is passed, it just does simulation.solution automatically. I am finding myself writing [sim1.solution, sim2.solution, etc] or having to do [sim.solution for sim in [sim1, sim2, sim3]] quite a bit, which is just messy

@valentinsulzer
Copy link
Member Author

valentinsulzer commented Mar 12, 2020

  • put in the colorbar
  • fix axis limits to be at 0 and x_max
  • dashed lines for subdomain boundaries
  • update time unit in labels
  • update spatial unit in labels

valentinsulzer added a commit that referenced this issue Mar 12, 2020
valentinsulzer added a commit that referenced this issue Mar 12, 2020
valentinsulzer added a commit that referenced this issue Mar 12, 2020
valentinsulzer added a commit that referenced this issue Mar 12, 2020
@valentinsulzer valentinsulzer mentioned this issue Mar 12, 2020
8 tasks
valentinsulzer added a commit that referenced this issue Mar 13, 2020
valentinsulzer added a commit that referenced this issue Mar 13, 2020
valentinsulzer added a commit that referenced this issue Mar 20, 2020
valentinsulzer added a commit that referenced this issue Mar 20, 2020
valentinsulzer added a commit that referenced this issue Mar 20, 2020
valentinsulzer added a commit that referenced this issue Mar 20, 2020
valentinsulzer added a commit that referenced this issue Mar 21, 2020
valentinsulzer added a commit that referenced this issue Mar 21, 2020
valentinsulzer added a commit that referenced this issue Mar 22, 2020
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 a pull request may close this issue.

3 participants