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

Fix EnsembleSummary plot recipe kwarg #697 #700

Merged

Conversation

jonathanfischer97
Copy link
Contributor

@jonathanfischer97 jonathanfischer97 commented May 28, 2024

In reference to #697:

  • Updated the trajectories kwarg to idxs in the EnsembleSummary recipes for both Plots and Makie.
  • Should now allow users to select which variables to show summary line series of, in line with what the documentation shows.

Tests

  • Wasn't sure where to put tests for this, or where plotting tests are kept. Should be something like:
idxs = (2, 3)
ensemble_summary_plot = plot(ensemble_summary; idxs)
@test ensemble_summary_plot.n == length(idxs)

Further considerations or to-dos

  1. Was the original intention of the EnsembleSummary recipes to allow users to choose/truncate the amount of trajectories that statistics are plotted over, similar to EnsembleSimulation? (Explains the mixup with idxs)

    • If so, that functionality is missing and should be added
  2. Symbolic indexing does not work any of the recipes for either EnsembleSimulation or EnsembleSummary.

    • Should be added to match the recipe of AbstractSolution.
  3. Plotting state spaces via giving idxs as a Tuple rather than a Vector does not work.

    • Should also be added to match the non-ensemble solution plotting recipes, if intended by the maintainers.
  4. Makie EnsembleSummary recipe doesn't show transparent ranges:
    image
    Shown are 100 trajectories.

  5. No user-exposed way to set the color map of EnsembleSimulation trajectories? Would like an easy way to set it to something other than random, like this:

image

`EnsembleSummary`'s plot recipe refered to the variable indices as `trajectories`, likely typo from the `EnsembleSimulation` recipe above where that nomenclature made sense. 

Fixed it to use `idxs`, allowing users to select which variables to plot the summaries of, in line with the documentation.
`EnsembleSummary`'s plot recipe refered to the variable indices as `trajectories`, likely typo from the `EnsembleSimulation` recipe above where that nomenclature made sense. 

Fixed it to use `idxs`, allowing users to select which variables to plot the summaries of, in line with the documentation.
@ChrisRackauckas ChrisRackauckas merged commit de51914 into SciML:master May 29, 2024
6 of 10 checks passed
@jonathanfischer97 jonathanfischer97 deleted the fix_ensemblesummary_plot branch May 29, 2024 14:22
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.

2 participants