convert metric plots to plotly #359

lboeman · 2020-03-10T22:35:36Z

~~Closes #xxxx .~~
I am familiar with the contributing guidelines.
Tests added.
Updates entries to docs/source/api.rst for API changes.
Adds descriptions to appropriate "what's new" file in docs/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

Bokeh's interactivity and loading are becoming an issue for larger reports. This converts all metrics plots to plotly. The motivations for this switch are as follows:

Plotly has a mature and well-documented javascript library. This means that we don't have to rely entirely on the product of some python function call to alter plots. While this may be possible in Bokeh, documentation does not exist or is insufficient.
Plotly has a declarative json schema (https://help.plot.ly/json-chart-schema/) which makes storage of these plots convenient. The json string can be read in by either the python or javascript library to reproduce plots. This also saves us from having to store <script> elements.

The current code emulates the existing Bokeh plots closely, but there are a couple of things that need to be resolved before this is merged.

CSV downloads need to be re-implemented. Each plot now contains it's own data, and we've lost the ability to access the ColumnDataSource on the front end, perhaps this is just accomplished by parshing and formatting the metrics on the report.
Adapt the parsing of the plotly JSON such that it works for both downloads and rendering in browser, while still achieving the initial intention of lazy-loading the plots after page load.

wholmgren · 2020-03-10T23:05:26Z

This also saves us from having to store <script> elements.

and this presents a security risk to end users that are sharing reports with each other, right?

CSV downloads need to be re-implemented. Each plot now contains it's own data, and we've lost the ability to access the ColumnDataSource on the front end, perhaps this is just accomplished by parshing and formatting the metrics on the report.

Should we make an API end point for this?

Adapt the parsing of the plotly JSON such that it works for both downloads and rendering in browser, while still achieving the initial intention of lazy-loading the plots after page load.

have you done any experiments to determine if this is actually going to make a difference in page loading?

lboeman · 2020-03-10T23:27:31Z

and this presents a security risk to end users that are sharing reports with each other, right?

Yes, this makes it more foolproof to ensure we're inserting a malicious script into a template.

Should we make an API end point for this?

If it would be useful beyond this single button, then sure we could add one. Otherwise following the js route seems simpler.

have you done any experiments to determine if this is actually going to make a difference in page loading?

I haven't yet, but the intention here would be to first load the entire document, and then begin inserting metric plots into the correct divs. So we may not end up with a speedup in a completely rendered document, but shifting any long running rendering to the end of the line should improve things.

wholmgren · 2020-03-12T15:45:58Z

solarforecastarbiter/datamodel.py

+def __check_plot_spec__(plot_spec):
+    """Ensure that the provided plot specification is valid JSON"""
+    try:
+        json.loads(plot_spec)


maybe https://python-jsonschema.readthedocs.io/en/stable/ instead?

That's an interesting implementation of a 'JSON schema' parser. It doesn't seem to enforce the very opinionated specification proposed by JSON Schema .

From tinkering with this briefly, it looks like we'd still have to load the json string into a python dict and then call validate(instance=json_dict, schema={"type":"object"}) to use this effectively. It does look like it'll give us extra safety though, so that slight performance hit may be worth it?

wholmgren · 2020-03-12T15:48:52Z

solarforecastarbiter/reports/figures.py

@@ -313,7 +336,7 @@ def scatter(timeseries_value_cds, timeseries_meta_cds, units):
    return fig


-def construct_metrics_cds(metrics, rename=None):
+def construct_metrics_dataframe(metrics, rename=None):


Instead of deleting the existing code, we could create figures/plotly.py and figures/bokeh.py. Maybe not really needed but multiple plotting "backends" could be useful at some point.

I think that's a really good idea. We could add logic to select the processing function based on the sfa reports version. Might require some minor tweaking to the bokeh handling, but I think that'd be a good idea.

lboeman · 2020-03-12T20:45:09Z

Added the reports.figures module and within it the bokeh_figures.py and plotly_figures.py. I've adjusted the reports code to only use plotly_figures.py and leave the bokeh code there for adventurous core users. Do the changes there seem reasonable or am I off mark on that?

I've also added a script to insert the plotly plots when the standalone html file is generated. Otherwise on the dashboard, the metric_plots variable contains the json to create each plot and the appropriate divs for insertion.

wholmgren

I'm confused. I thought we'd put bokeh stuff in a bokeh module and plotly stuff in a plotly module.

wholmgren · 2020-03-12T20:46:24Z

docs/source/api.rst

+   reports.figures.plotly_figures.timeseries_plots
+
+
+Retired functions for generating Bokeh metric plots.


Suggested change

Retired functions for generating Bokeh metric plots.

Functions for generating Bokeh metric plots.

still using time series

wholmgren · 2020-03-12T20:51:09Z

solarforecastarbiter/reports/figures/bokeh_datamodel.py

+"""File containing necessary datamodel objects for creating bokeh metric
+plots.


Suggested change

"""File containing necessary datamodel objects for creating bokeh metric

plots.

"""File containing datamodel objects for creating bokeh metric plots.

wholmgren · 2020-03-12T20:52:15Z

solarforecastarbiter/datamodel.py

+    except (json.JSONDecodeError, ValidationError):
+        raise ValueError('Figure spec must be a valid json object.')
+
+
 @dataclass(frozen=True)
 class ReportFigure(BaseModel):


PlotlyReportFigure? Doesn't seem like a library-agnostic class.

I think I misunderstood earlier, are you opposed to having an empty ReportFigure parent class that gets sub classed by BokehReportFigure and PlotlyReportFigure? I think I mistook your opposition to one class with optional fields as being opposed to that as well.

are you opposed to having an empty ReportFigure parent class that gets sub classed by BokehReportFigure and PlotlyReportFigure?

I am not opposed to that. It sounds reasonable. @alorenzo175 should comment before I waste more of your time.

sub-classes are ok, but we should make sure they include a type key so that it's easier to load into BokehReportFigure or PlotlyReportFigure when needed

wholmgren · 2020-03-12T20:54:04Z

solarforecastarbiter/datamodel.py

    svg: str
    figure_type: str
    category: str = ''
    metric: str = ''

+    def __post_init__(self):
+        __check_plot_spec__(self.spec)
+

 @dataclass(frozen=True)
 class RawReportPlots(BaseModel):


PlotlyRawReportPlots? (as above)

wholmgren · 2020-03-12T20:55:35Z

solarforecastarbiter/reports/figures/bokeh_figures.py

+This code is currently unreachable from the rest of the Solar Forecast Arbiter
+Core library. It may be used in place of the plotly_figures to generate bokeh


isn't the time series still used?

wholmgren · 2020-03-12T20:57:01Z

solarforecastarbiter/reports/figures/plotly_figures.py

+from bokeh.embed import components
+from bokeh.layouts import gridplot
+from bokeh.models import ColumnDataSource, Legend, CDSView, BooleanFilter
+from bokeh.models.ranges import Range1d
+from bokeh.plotting import figure
+from bokeh import palettes


no bokeh stuff in the plotly module

wholmgren · 2020-03-13T19:07:56Z

CSV downloads need to be re-implemented. Each plot now contains it's own data, and we've lost the ability to access the ColumnDataSource on the front end, perhaps this is just accomplished by parshing and formatting the metrics on the report.
Adapt the parsing of the plotly JSON such that it works for both downloads and rendering in browser, while still achieving the initial intention of lazy-loading the plots after page load.

@lboeman seems that the 2nd is now done but not the first?

lboeman · 2020-03-13T19:11:20Z

@wholmgren Yes, and I found a bug in the plotting that I'm trying to rectify. You said that we don't necessarily need to display all months, or all days of the week for reports spanning only a few months or days correct?
For csv downloads I think I recall that you wanted to format the csv the same way that we are displaying the metrics tables in reports?

wholmgren · 2020-03-13T19:17:06Z

You said that we don't necessarily need to display all months, or all days of the week for reports spanning only a few months or days correct?

Hours plots should always have 0-24.

Day of week should always have 7 days.

I'm ok with other plots only showing periods for which there is actually data.

For csv downloads I think I recall that you wanted to format the csv the same way that we are displaying the metrics tables in reports?

Seems like a good approach but if it's difficult then we could look at other options.

lboeman · 2020-03-13T21:11:32Z

So it turns out to be a pain to get plotly to display all of the tick labels you supply for a 'categorical' variable. As of this last push, hour of day displays as expected due to the numeric labels. However, the day of the week plots end up like this for, e.g. missing data on saturday

There are just missing ticks for the data that should be there.

lboeman · 2020-03-16T18:45:58Z

Fixed the weekly plots so that they display a full week regardless of available data. as below:

CSV downloads have been implemented by dumping the metrics dataframe as json, and loading it into the html.

wholmgren · 2020-03-16T21:05:18Z

solarforecastarbiter/reports/figures/plotly_figures.py

+            # week of data.
+            y_values = [plot_data[plot_data['index'] == day]['value'].iloc[0]
+                        if not plot_data[plot_data['index'] == day].empty
+                        else 0 for day in x_ticks]


can we use nan instead of 0?

will the same problem occur with hourly if we exclude nighttime values?

The hourly plots work correctly when dropping nighttime values. My test report excludes them. For some reason, plotly respects the x_range of an axis when provided as a series of numerical values, but not for things it decides are categorical.

solarforecastarbiter/datamodel.py

solarforecastarbiter/reports/figures/bokeh_datamodel.py

solarforecastarbiter/reports/figures/plotly_figures.py

solarforecastarbiter/datamodel.py

solarforecastarbiter/reports/figures/tests/test_plotly_figures.py

solarforecastarbiter/reports/figures/plotly_figures.py

solarforecastarbiter/reports/figures/tests/test_plotly_figures.py

lboeman · 2020-03-19T16:17:01Z

I've opened an issue for adding pdf field to the datamodel.
I believe I've hit all of your collective comments and this is ready for merge. I'll have my full attention back on work on Monday if we'd like to wait for me to be more available to put out any PR related fires. The SolarArbiter/solarforecastarbiter-dashboard#208 PR should display reports adequately, and reports will need to be recomputed to work with this PR.

alorenzo175

looking pretty good. some minor things

alorenzo175 · 2020-03-23T18:11:54Z

solarforecastarbiter/datamodel.py

+    spec: str
+    svg: str
+    figure_type: str
+    figure_class: str = 'plotly'


Move this last and possibly make it a field(init=False) so users can't set it

alorenzo175 · 2020-03-23T18:23:41Z

solarforecastarbiter/reports/templates/head.html

+{% if plotly_version %}
+{# plotly js and the python plotly library do not have matching versions #}
+{% include "insert_plots.html" %}
+<script src="https://cdn.plot.ly/plotly-1.52.3.min.js"></script>


is there any reason to record plotly_version in the report then?

It seems like it might be useful to debug any issues that arise from an older report and newer core library. But maybe just recomputing is the answer there.

alorenzo175 · 2020-03-23T18:33:53Z

solarforecastarbiter/reports/figures/plotly_figures.py

+        svg = fig.to_image(format='svg').decode('utf-8')
+    except Exception:
+        logger.error('Could not generate SVG for figure %s',
+                     getattr(fig, 'name', 'unnamed'))


I guess plotly figures don't have name? can you replace name with something that identifies the figure?

alorenzo175 · 2020-03-23T18:41:38Z

solarforecastarbiter/reports/figures/plotly_figures.py

+        for mvalue in metric_result.values:
+            new = {
+                'name': metric_result.name,
+                'abbrev': f(metric_result.name),


any idea why the abbreviate column is the same as name when I run the test report from the cli?

I had neglected to pass the abbreviate function when generating the metrics_json template var in template.py.

…gure title, pass abbreviation function when generating metrics_json object for csv download

…it, update report test

lboeman · 2020-03-23T20:14:01Z

@alorenzo175 Think I've hit all your comments. I'm not really opposed to removing plotly_version from the report figures, but I think it might be helpful in debugging down the road if we want to do our own version matching between plotly's js library and python.

alorenzo175 · 2020-03-23T22:36:28Z

solarforecastarbiter/datamodel.py

@@ -1255,10 +1257,9 @@ def from_dict(model, input_dict, raise_on_extra=False):
        dict_ = input_dict.copy()
        if model != ReportFigure:
            return super().from_dict(dict_, raise_on_extra)
-        figure_class = dict_.get('figure_class')
-        if figure_class == 'plotly':


eee, maybe making init false was a bad idea. I prefer using the figure class to determine this, so probably best to remove that, but keep the parameter at the end

alorenzo175

do we need to wait for the dashboard?

lboeman · 2020-03-23T23:21:18Z

do we need to wait for the dashboard?

Dashboard PR SolarArbiter/solarforecastarbiter-dashboard#208 should work with this PR. Its tests should pass after this is merged. Any existing reports will need to be recomputed. I can take on the API issue to add a recompute endpoint so it'll be easier to roll this out into prod later if you're not already working on it.

alorenzo175 · 2020-03-23T23:23:09Z

if you're not already working on it.

I'm not

convert metric plots to plotly

6f9cea8

lboeman added 5 commits March 11, 2020 16:25

add plotly, psutil to requirements

e8bca4c

update datamodel, tests

c0de0e6

fix offset, None = no offset, 0=left justified

59bcbe2

flake8

3e73af6

script to wait for load event to build metrics plots

df410b9

wholmgren reviewed Mar 12, 2020

View reviewed changes

lboeman added 6 commits March 12, 2020 09:46

utilize jsonschema to enforce json is of type object

5b7380e

create figures directory for separate plotting back ends

c3a2984

flake8 datamodel

de38eaf

adjust tests and datamodel for figures module

18a9f6f

update some docs, remove bokeh svg generation from plotly_figures

a1265df

remove console.log calls

f6c658e

lboeman marked this pull request as ready for review March 12, 2020 20:45

wholmgren reviewed Mar 12, 2020

View reviewed changes

lboeman added 2 commits March 12, 2020 14:13

separate bokeh and plotly code into each module

57cb43b

update datamodel with BokehReportFigure and PlotlyReport figure

98ca366

lboeman mentioned this pull request Mar 13, 2020

add logic to load plots only when shown in metrics section SolarArbiter/solarforecastarbiter-dashboard#208

Merged

declare plotly version

ba28fca

actually plot the correct data in subdivided bar plots

a9c0ba5

lboeman added 2 commits March 13, 2020 15:32

update download as csv

2974fe5

Fill weekday data to force display of all weekdays

8d422eb

update whatsnew, tests

feacab4

lboeman requested a review from alorenzo175 March 16, 2020 19:38

wholmgren reviewed Mar 16, 2020

View reviewed changes

alorenzo175 reviewed Mar 16, 2020

View reviewed changes

lboeman added 4 commits March 16, 2020 15:58

remove remaining bokeh code from plotly module

93afa38

remove dict from error message in from_dict functions

fbc13c2

fill with nan instead of zero when filling weekday plot data

a127691

add note and link to orca documentation for plotly

eb7abd5

lboeman mentioned this pull request Mar 19, 2020

Add plotly to pdf functionality #360

Closed

update report figure fixtures and hit missing datamodel code

fd9e730

lboeman requested a review from alorenzo175 March 23, 2020 16:19

flake8

9712fc7

alorenzo175 reviewed Mar 23, 2020

View reviewed changes

lboeman added 2 commits March 23, 2020 12:28

pass correct values to output_svg, update error message to include fi…

4cd1580

…gure title, pass abbreviation function when generating metrics_json object for csv download

set init to false for figure class fields to keep users from setting …

0c04edc

…it, update report test

alorenzo175 reviewed Mar 23, 2020

View reviewed changes

don't set init false on figure_class

ab0d951

alorenzo175 approved these changes Mar 23, 2020

View reviewed changes

alorenzo175 merged commit ee053b9 into SolarArbiter:master Mar 23, 2020

wholmgren added this to the 1.0 beta 5 milestone Mar 24, 2020

wholmgren added the enhancement New feature or request label Mar 24, 2020

wholmgren mentioned this pull request Mar 31, 2020

use plotly to make observation and forecast page plots #371

Open

lboeman mentioned this pull request Apr 27, 2020

Improve report loading and performance for large reports SolarArbiter/solarforecastarbiter-dashboard#206

Closed

lboeman deleted the plotly branch August 16, 2021 19:52

dplarson mentioned this pull request Mar 28, 2023

Remove psutil after orca removal #813

Open

		reports.figures.plotly_figures.timeseries_plots


		Retired functions for generating Bokeh metric plots.

	Retired functions for generating Bokeh metric plots.
	Functions for generating Bokeh metric plots.

		"""File containing necessary datamodel objects for creating bokeh metric
		plots.

	"""File containing necessary datamodel objects for creating bokeh metric
	plots.
	"""File containing datamodel objects for creating bokeh metric plots.

		This code is currently unreachable from the rest of the Solar Forecast Arbiter
		Core library. It may be used in place of the plotly_figures to generate bokeh

convert metric plots to plotly #359

convert metric plots to plotly #359

Conversation

lboeman commented Mar 10, 2020 • edited by wholmgren Loading

wholmgren commented Mar 10, 2020

lboeman commented Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

lboeman Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lboeman commented Mar 12, 2020

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren commented Mar 13, 2020

lboeman commented Mar 13, 2020

wholmgren commented Mar 13, 2020 • edited Loading

lboeman commented Mar 13, 2020

lboeman commented Mar 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lboeman commented Mar 19, 2020

alorenzo175 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lboeman commented Mar 23, 2020

Choose a reason for hiding this comment

alorenzo175 left a comment

Choose a reason for hiding this comment

lboeman commented Mar 23, 2020

alorenzo175 commented Mar 23, 2020

lboeman commented Mar 10, 2020 •

edited by wholmgren

Loading

lboeman commented Mar 10, 2020 •

edited

Loading

lboeman Mar 12, 2020 •

edited

Loading

wholmgren commented Mar 13, 2020 •

edited

Loading