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

Explore Simulation #518

Open
wants to merge 46 commits into
base: master
from

Conversation

@lrennels
Copy link
Collaborator

commented Jun 4, 2019

Issue #477

@lrennels lrennels added the enhancement label Jun 4, 2019

@lrennels lrennels self-assigned this Jun 4, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jun 5, 2019

Codecov Report

Merging #518 into master will decrease coverage by 2.56%.
The diff coverage is 1.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #518      +/-   ##
==========================================
- Coverage   81.18%   78.62%   -2.57%     
==========================================
  Files          26       26              
  Lines        1977     1988      +11     
==========================================
- Hits         1605     1563      -42     
- Misses        372      425      +53
Impacted Files Coverage Δ
src/Mimi.jl 100% <ø> (ø) ⬆️
src/explorer/explore.jl 45.45% <0%> (-29.55%) ⬇️
src/explorer/buildspecs.jl 37.12% <2.43%> (-17.52%) ⬇️
src/core/connections.jl 82.23% <0%> (-7.82%) ⬇️
src/core/model.jl 45.97% <0%> (-2.3%) ⬇️
src/core/defs.jl 91.95% <0%> (-0.38%) ⬇️
src/core/defcomp.jl 89.44% <0%> (-0.07%) ⬇️
src/core/build.jl 98.86% <0%> (-0.02%) ⬇️
src/mcs/mcs_types.jl 77.5% <0%> (+1.89%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6339f0...54cfab8. Read the comment docs.

@lrennels

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 9, 2019

Note that need to update for the new sim API and pre-calculate statistics for trumpet plots

@rjplevin

This comment has been minimized.

Copy link
Collaborator

commented on src/mcs/montecarlo.jl in f2cc944 Jun 13, 2019

You might reduce redundancy a bit by distilling the part that depends on scen_name:

pairs = Any[typeof(value) => value, :trialnum => trialnum]
has_scen && push!(pairs, :scen => scen_name)
trial_df = DataFrame(pairs)

This comment has been minimized.

Copy link
Collaborator Author

replied Jun 13, 2019

Good idea!

@rjplevin

This comment has been minimized.

Copy link
Collaborator

commented on src/mcs/montecarlo.jl in f2cc944 Jun 13, 2019

if results_output_dir !== nothing

@rjplevin

This comment has been minimized.

Copy link
Collaborator

commented on src/mcs/montecarlo.jl in f2cc944 Jun 13, 2019

streams = Dict{String, IOStream} assigns the type. You need streams = Dict{String, IOStream}()

@rjplevin

This comment has been minimized.

Copy link
Collaborator

commented on src/mcs/montecarlo.jl in f2cc944 Jun 13, 2019

I think we should rename this _save_trial_results since it's only called from _store_trial_results and can't really be called outside all the context built up here.

@lrennels

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 18, 2019

Some TODOs:

  • pre-filter data for min/max/mean before passing to VegaLite ... and come up with similar method for histograms
  • add methods for specifying an output_dir
@lrennels

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 27, 2019

UPDATE: I fixed the below by updating to the latest javascript files. Now working on some other issues this created ...

  • the div that holds the plot now appears below the menu list instead of to the right
  • the transform to filter interactive plots in explore(model) doesn't work now, only the scale way we do it in explore(sim) but that's not ideal for the former ... both now use domain but transform would be ideal
<script src="https://cdn.jsdelivr.net/npm/vega@3"></script>
<script src="https://cdn.jsdelivr.net/npm/vega-lite@2"></script>
<script src="https://cdn.jsdelivr.net/npm/vega-embed@3"></script>

original post

davidanthoff I'm having trouble debugging why calls to plot are working for my trumpet plots, but calls to explore are not displaying the plot properly. Since this discrepancy isn't a problem with explore(model) and only explore(sim), I'm thinking it has to do with the syntax of these plots which layer one x axis with a a line plot and an area plot and how they are treated in the different calls.

Both use the exact same function to get the spec (Mimi._spec_for_sim_item), which holds the full spec. The plot function then calls return VegaLite.VLSpec{:plot}(spec[:VLspec]) where :VLspechas the actual specification. In contrast, explore passes the full JSONified JSON.json(spec) to a Javasript function display, which has the signature:

function display(spec) {
    vegaEmbed("#vis", spec["VLspec"], {actions: false});
}

For some reason, however, plot displays things perfectly, while explore has trouble rendering the full plot. Are there differences that you know of between VegaLite. vegaEmbed and VegaLite.jl's VegaLite.VLSpec{:plot} that could cause a difference here? I'm a bit stuck ... advice on even how to further debug would also be appreciated :)

@lrennels

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 27, 2019

Also to handle ...

  • regional trumpet plots overflow the div
  • regional trumpet plot interactivity overlaps between graphs ...
  • histograms and regional histograms

lrennels added some commits Jun 27, 2019

@lrennels lrennels changed the title WIP Explore Simulation Explore Simulation Jun 28, 2019

@lrennels

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 28, 2019

@rjplevin if you get the chance take a look at this PR and let me know if you have any thoughts. It's a first go and an explore(sim_inst::SimulationInstance) function and the partner plot functions. I'll write some documentation and graphics soon, but in essence there are 4 types of plots:

  1. trumpet plots
  2. paneled trumpet plots
  3. histograms
  4. paneled histogram plots

All of these are displayed if you look to test_explorer_sim. Both plot and explore allow a user to input a model_index (defaults to 1) and requires the user to input a scen_name if there are scenarios. This currently all requires that the data is stored on disk in sim_inst.results. I'll probably add functions where you can pass an output_dir at some point since we do know what that folder structure should look.

One Q: for the histograms do you think the top summary graphic should have overlapping histograms, stacked histograms, or something else?

@lrennels lrennels requested a review from rjplevin Jun 28, 2019

@rjplevin
Copy link
Collaborator

left a comment

There's a lot of good stuff here! Nice work.

@lrennels

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 28, 2019

  • documentation
  • output_dir argument methods
  • the transform to filter interactive plots in explore(model) doesn't work now, only the scale way we do it in explore(sim) but that's not ideal for the former ... both now use domain but transform would be ideal

lrennels added some commits Jun 29, 2019

@lrennels lrennels requested a review from davidanthoff Jul 9, 2019

lrennels added some commits Jul 10, 2019

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