Add interactive plots with Plotly by javierbg · Pull Request #163 · glotzerlab/signac-dashboard

javierbg · 2022-11-25T11:34:48Z

Description

This pull request adds a new module to the dashboard for showing interactive plots in the browser.

Motivation and Context

See issue #162

Checklist:

I am familiar with the Contributing Guidelines.
I agree with the terms of the Contributor Agreement.
The changes introduced by this pull request are covered by existing or newly introduced tests.
The package documentation and framework documentation in signac-docs are up to date with these changes.
I have updated the changelog and added any related issue and pull request numbers for future reference.

javierbg · 2022-11-25T11:36:47Z

Do you consider that a new test is needed for the module?

bdice

@javierbg Thanks for the pull request! The code looks solid but I haven't tested it yet. This seems like a great feature to include in the dashboard examples. Can you add an example there? Then I'll re-review and test the example on my machine.

changelog.txt

bdice · 2022-11-27T03:54:37Z

doc/api.rst

    modules.StatepointList
    modules.TextDisplay
    modules.VideoViewer
+    modules.PlotViewer


Alphabetize this in the list.

bdice · 2022-11-27T03:54:47Z

signac_dashboard/modules/__init__.py

    "StatepointList",
    "TextDisplay",
    "VideoViewer",
+    "PlotViewer",


Alphabetize this in the list.

signac_dashboard/modules/plot_viewer.py

bdice · 2022-11-27T04:01:33Z

signac_dashboard/modules/plot_viewer.py

+        dashboard.app.route("/module/plot_viewer/<path:filename>")(plot_viewer_asset)
+
+        # Register assets
+        assets = ["js/plot_viewer.js", "js/plotly-2.16.1.min.js"]


This asset is pretty large and it seems like it changes often (could get outdated, at the relatively slow pace of signac-dashboard development). Can we use the CDN instead?

Note that you'll need to rewrite the git history on this branch to prevent the inclusion of this file in the git history. If you need help with this task or are uncertain about how to do it correctly, let me know and I'll take care of it!

Suggested change

assets = ["js/plot_viewer.js", "js/plotly-2.16.1.min.js"]

assets = ["js/plot_viewer.js", "https://cdn.plot.ly/plotly-2.16.1.min.js"]

Well, I have never done it before but I can give it a try. I'll let you know if I need any help. It should not be a problem to commit your changes first, right? (by the way, is it ok if I batch the smaller ones?)

I'm happy to make this change, the asset is definitely very large, but maybe it's not so popular to have to rely on an external service for this module. Maybe a solution where the user can provide a local copy as a fallback would be useful? I guess it would be a niche situation and not work the effort, but I thought I'd mention it.

You can commit GitHub suggestions individually or batched. Your decision!

Maybe a solution where the user can provide a local copy as a fallback would be useful? I guess it would be a niche situation and not work the effort, but I thought I'd mention it.

Yeah, I don't think we are intending for signac-dashboard to be fully usable offline. The core features are baked in, but I don't think it's necessary to include large assets for specific modules like this one in the distributed package. It increases the size of the signac-dashboard repo and downloaded package, and has a high likelihood of going out of date.

@javierbg to rewrite the history of the branch perform an "interactive rebase" and remove the commits that added the large file.

The dashboard developers could alternately squash merge this branch to prevent the large file addition to the history.

signac_dashboard/templates/cards/plot_viewer.html

signac_dashboard/templates/plot_viewer/js/plot_viewer.js

codecov · 2022-11-27T04:11:39Z

Codecov Report

Merging #163 (24a9aa2) into master (21c3cb7) will decrease coverage by 0.19%.
The diff coverage is 70.58%.

❗ Current head 24a9aa2 differs from pull request most recent head 121578f. Consider uploading reports for the commit 121578f to get more accurate results

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
- Coverage   73.83%   73.64%   -0.20%     
==========================================
  Files          20       21       +1     
  Lines         795      846      +51     
==========================================
+ Hits          587      623      +36     
- Misses        208      223      +15

Impacted Files	Coverage Δ
signac_dashboard/modules/plotly_viewer.py	`70.00% <70.00%> (ø)`
signac_dashboard/modules/__init__.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

bdice · 2022-11-27T04:12:12Z

Do you consider that a new test is needed for the module?

Explicit tests aren't needed -- the testing framework is pretty weak right now, and this should be automatically covered by the "kitchen sink" tests. However, an example would be good to include.

Charlottez112 · 2022-11-27T15:47:03Z

I agree with Bradley. This seems very useful. Thank you for the implementation! If you could include an example, I'll also test it locally on my machine.

javierbg

Minor changes for #163

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

javierbg · 2022-11-28T09:45:24Z

I'm not familiarized with Flask, so when it came to add the routes and assets I just used other modules as reference (I think it was the DocumentEditor module).

When adding the Plotly CDN URL I realized that the "filename" attribute in line 104 was not used, apparently, so I removed it. Is this ok?

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

joaander

Thanks for the addition. However, you need to protect access to the new routes with flask_login.

I'm not able to add a code suggestion for this as your module does not add routes with the same pattern as the others. See the changes in #158 for examples.

joaander · 2022-11-28T16:22:52Z

signac_dashboard/modules/plot_viewer.py

+
+
+def plot_viewer_asset(filename):
+    try:


Protect this route from access by unauthenticated users.

joaander

Thanks! That should work to protect the new route.

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

examples/plots-plotly/dashboard.py

signac_dashboard/modules/plot_viewer.py

bdice · 2022-11-28T19:00:45Z

signac_dashboard/templates/plot_viewer/js/plot_viewer.js

@@ -0,0 +1,12 @@
+function draw_plot(element) {
+    data = JSON.parse(element.getAttribute("data-plotly-data"));


Maybe the card HTML can contain a REST endpoint, and fetch the data with a second call? That way we don't have to generate and include all the raw data in the HTML page. This should help a lot with page responsiveness. You can see examples of REST calls in other modules:

https://github.com/glotzerlab/signac-dashboard/blob/master/signac_dashboard/templates/notes/js/notes.js

https://github.com/glotzerlab/signac-dashboard/blob/master/signac_dashboard/templates/document_editor/js/document_editor.js

If you need help with this, let me know.

I have implemented this in 226111e. The page loading is way more responsive now.

Still, I'm a total newbie with Flask, so a thorough review is needed.

My main concerns are:

I have had to create a module identifier so that a different endpoint is created for each instance of the PlotlyView module. I decided to hash the id(self) which should work, but has the disadvantage that each time the dashboard is launched the URLs change. I don't think this is a huge problem, but I feel like there is a better way to do this.

For each added instance of the module, the register method is called. However, the assets URLs should only be added once. If you try to do it multiple times, the second time around the module is not registered. I have solved this with a class attribute PlotlyViewer._assets_url_registered that avoids this duplication. Still, I feel like this solution is somewhat hacky.

@javierbg
Flask Blueprints are the way to go. I started experimenting with them in #182 to allow multiple Notes modules to be added.

Now instead of passing the data as HTML attributes a REST endpoint has been set up so that the page is rendered quickly and the client can fetch the data with a GET request

signac_dashboard/modules/plotly_viewer.py

The Project.id and Job.id properties were not available in signac 1.0.0. The fix changes it to use the get_id() method in both cases.

for more information, see https://pre-commit.ci

javierbg requested review from a team as code owners November 25, 2022 11:34

javierbg requested review from Charlottez112 and tcmoore3 and removed request for a team November 25, 2022 11:34

bdice reviewed Nov 27, 2022

View reviewed changes

javierbg commented Nov 28, 2022

View reviewed changes

javierbg added a commit to javierbg/signac-dashboard that referenced this pull request Nov 28, 2022

Minor revisions for glotzerlab#163

37655c1

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

javierbg force-pushed the feature/issue-162-plotly-plots branch from c55b48f to a3afa94 Compare November 28, 2022 11:58

javierbg requested a review from a team as a code owner November 28, 2022 11:58

javierbg added a commit to javierbg/signac-dashboard that referenced this pull request Nov 28, 2022

Minor revisions for glotzerlab#163

5c5b356

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

javierbg force-pushed the feature/issue-162-plotly-plots branch from a3afa94 to c55b48f Compare November 28, 2022 11:59

joaander requested changes Nov 28, 2022

View reviewed changes

javierbg requested review from joaander and removed request for Charlottez112 and tcmoore3 November 28, 2022 16:37

joaander reviewed Nov 28, 2022

View reviewed changes

javierbg added 7 commits November 28, 2022 12:23

Add plot viewer module

0a50ca2

Add documentation for PlotViewer module

23bdaaa

Move plotly to module asset

73d0ee4

Extend PlotViewer documentation

406710d

Add PlotViewer to the changelog

ba7edd8

Remove unnecesary id from plot viewer div

954a603

Remove unnecesary console log

b4985b5

bdice added a commit to javierbg/signac-dashboard that referenced this pull request Nov 28, 2022

Minor revisions for glotzerlab#163

5c51ca3

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

bdice force-pushed the feature/issue-162-plotly-plots branch from 839d41b to 6ebebf9 Compare November 28, 2022 18:25

javierbg and others added 6 commits November 28, 2022 12:51

Minor revisions for glotzerlab#163

c971876

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

Alphabetize PlotViewer module in lists

74c3ce2

Use Plotly from CDN instead of local copy

2e7aea5

Rename Matplotlib plot example

d62d08a

Add Plotly example

59b37bc

Protect access to PlotViewer routes

a898191

bdice force-pushed the feature/issue-162-plotly-plots branch from 6ebebf9 to a898191 Compare November 28, 2022 18:52

bdice reviewed Nov 28, 2022

View reviewed changes

Javier Barbero and others added 5 commits November 28, 2022 20:26

Unify Matplotlib and Plotly examples

02df95b

Rename PlotViewer to PlotlyViewer

e4bd48d

Simplify module registration for PlotlyViewer

d271eee

Move PlotlyView argument passing functionality to REST endpoint

226111e

Now instead of passing the data as HTML attributes a REST endpoint has been set up so that the page is rendered quickly and the client can fetch the data with a GET request

Update PlotlyViewer docstring for new callable type

24a9aa2

joaander reviewed Nov 29, 2022

View reviewed changes

signac_dashboard/modules/plotly_viewer.py Show resolved Hide resolved

Javier Barbero and others added 4 commits November 29, 2022 15:47

Fix PlotlyViewer for old signac API

121578f

The Project.id and Job.id properties were not available in signac 1.0.0. The fix changes it to use the get_id() method in both cases.

Merge branch 'main' into feature/issue-162-plotly-plots

152d94a

Partial fix for getting job id

3c32907

[pre-commit.ci] auto fixes from pre-commit.com hooks

89a5c3d

for more information, see https://pre-commit.ci

cbkerr self-assigned this Nov 9, 2023

	assets = ["js/plot_viewer.js", "js/plotly-2.16.1.min.js"]
	assets = ["js/plot_viewer.js", "https://cdn.plot.ly/plotly-2.16.1.min.js"]

		@@ -0,0 +1,12 @@
		function draw_plot(element) {
		data = JSON.parse(element.getAttribute("data-plotly-data"));

Conversation

javierbg commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Checklist:

Uh oh!

javierbg commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bdice commented Nov 27, 2022

Uh oh!

Charlottez112 commented Nov 27, 2022

Uh oh!

javierbg left a comment

Choose a reason for hiding this comment

Uh oh!

javierbg commented Nov 28, 2022

Uh oh!

joaander left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaander left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

javierbg commented Nov 25, 2022 •

edited

Loading

javierbg commented Nov 25, 2022 •

edited

Loading

codecov bot commented Nov 27, 2022 •

edited

Loading