Thumbnail fv_34 with JSDOM #79

ede0m · 2017-07-25T21:30:11Z

There is still plenty to do with this ticket. Namely, lots of refactoring once we start using reference.json. Right now we are using almost a direct copy of hydromodule, with a few minor differences to make this work post build. Ideally we would like to not have a duplicate file like this.

New dependencies:

npm install jsdom --save
npm install svg2png --save

Note: the flask freeze or python run command must be executed with THUMBNAIL=True in config.py before running the below command.

Usage:

node floodviz/thumbnail/thumbnail.js

…d to refactor the figure modules a bit to clean up this code

… have worked..

into thumbnail_FV_34

skaymen

Code could use additional commenting as mentioned in specific places.

Does not work correctly on my machine, but does work on two other machines. Possibly due to using an older version of node on my machine.

Overlapping PNGs is an issue but already being worked on.

skaymen · 2017-07-26T19:14:11Z

floodviz/posterior_build/hydro_thumbnail.js

@@ -0,0 +1,248 @@
+


Could use comments indicating this is same as hydrograph code and what differences are.

skaymen · 2017-07-26T19:18:38Z

floodviz/posterior_build/map_thumbnail.js

@@ -0,0 +1,363 @@
+


Could use similar comment to hydrograph.

skaymen · 2017-07-26T19:23:52Z

floodviz/posterior_build/thumbnail.js

@@ -0,0 +1,92 @@
+var fs = require('fs');


A comment here would help explain what these variables will be used for.

skaymen · 2017-07-26T19:24:39Z

floodviz/posterior_build/thumbnail.js

+		'./floodviz/posterioir_build/map_thumbnail.js'
+	],
+
+	function (err, window) {


Function comments would be helpful here

ede0m · 2017-07-26T20:16:16Z

I verified that this does not work with Node version 6.11.0. If we want to use this or a variant of this implementation, we will have to use node >= 7.5.

Below is the console error from using v6.11:

<--- Last few GCs --->

   31767 ms: Mark-sweep 1394.2 (1439.2) -> 1394.1 (1439.2) MB, 1274.7 / 0.0 ms (+ 0.9 ms in 6 steps since start of marking, biggest step 0.6 ms) [allocation failure] [scavenge might not succeed].
   33054 ms: Mark-sweep 1394.2 (1439.2) -> 1394.2 (1439.2) MB, 1285.4 / 0.0 ms (+ 0.6 ms in 1 steps since start of marking, biggest step 0.6 ms) [allocation failure] [scavenge might not succeed].


<--- JS stacktrace --->
Cannot get stack trace in GC.
FATAL ERROR: MarkCompactCollector: semi-space copy, fallback in old gen Allocation failed - JavaScript heap out of memory

EDIT

This heap memory error is a known issue in node:
see here
and here

EDIT-2

I believe I have isolated the problem to be hydrograph specific. The map png is outputted just fine when using v6.11.0. I believe it has something to do with the data file size when being passed to hydromodule.

EDIT-3

using a smaller hydrograph_data.json file lets the process go through. hmmmm

ede0m · 2017-07-27T19:59:50Z

Was just thinking that adding an optional argument for external css files may be useful. Let me know what you guys think and I can add this if desired.

mbucknell

Things to do before the code is merged.

The png's should not be committed and the other comments should be addressed. I think we could merge this than and then talk about next steps which might mean new tickets. For instance we need a ticket to address the Heap memory error. We need a ticket to actually code the build process in the Dockerfiles. There may be other things too. We should talk about this at sprint planning.

mbucknell · 2017-07-27T20:36:41Z

floodviz/posterior_build/hydro_thumbnail.js

@@ -0,0 +1,248 @@
+


How about calling the directory thumbnail

mbucknell · 2017-07-27T20:47:04Z

floodviz/posterior_build/hydro_thumbnail.js

+ * interaction functions for other figures should be passed to init in and object.
+ *
+ */
+var hydromodule = function (options) {


Could we change hydrograph.js not to be wrapped in a function? I'm not sure why we are doing that anyways. Then I think you could use it directly here

Are you saying change the original hydrograph.js to not be wrapped in a function, and then we wont have to replicate this file? I will definitely try this, but I am still thinking we will have to battle the FV.hydromodule issue with the original file.

Maybe. The alternate is to not use global namepsace of FV in the original module. I would rather do that the duplicate code.

Wondering if we could make another ticket for this and attack it once the reference.json changes are merged?

Actually, it looks like they are already merged. But a separate ticket for this would still be helpful IMO. There may be a decent amount of refactoring. Something to talk about at sprint planning I guess.

Let's talk about at sprint planning. I'm okay with putting in a separate ticket though.

mbucknell · 2017-07-27T20:49:13Z

floodviz/posterior_build/thumbnail.js

+						'width': 560,
+						'div_id': '#hydrograph',
+						'data': data_hydro,
+						"display_ids": ['05471200', '05476750', '05411850', '05454220',


These will have to be parameterized eventually.

mbucknell · 2017-07-27T20:53:29Z

floodviz/posterior_build/thumbnail.js

+	// load local assets into window environment
+	[
+		'./floodviz/static/bower_components/d3/d3.js',
+		'./floodviz/static/bower_components/jquery/dist/jquery.min.js',


Does any of this use jquery?

mbucknell · 2017-07-27T21:00:47Z

floodviz/views.py

@@ -34,6 +34,10 @@ def timeseries_data():
    # Hydrodata data clean and write
    j = hydrograph_utils.req_hydrodata(sites, hydro_start_date, hydro_end_date, url_nwis_prefix)
    timeseries_data = hydrograph_utils.parse_hydrodata(j)
+
+    # Write data to file for post build thumbnail
+    with open('floodviz/static/data/hydrograph_data.json', 'w') as f:


This should not be written to the static directory because it is not data used by the web site. For now let's write it to the new floodviz/thumbnail directory.

I'd also like this to happen only when we ask it to be generated. maybe through a config.py variable

mbucknell · 2017-07-27T21:06:48Z

floodviz/views.py

@@ -94,4 +98,8 @@ def _map_helper():
            ]
        }
    })
+
+    with open('floodviz/static/data/map_data.json', 'w') as f:


Same comments as above.

…od-viz into thumbnail_FV_34

mbucknell

Please address comments. In particular, remove code for the map and the css file in the thumbnail directory.

mbucknell · 2017-07-31T13:44:39Z

floodviz/thumbnail/hydro_thumb.css

@@ -0,0 +1,8 @@
+.hydro-inactive{


Is this to prove that a secondary css file can be brought in? If so, it should not be committed.

mbucknell · 2017-07-31T13:50:49Z

floodviz/thumbnail/map_thumbnail.js

+
+'use strict';
+/**
+ * @param {Object} options - holds options for the configuration of the map.


I'd like to limit the scope to only the hydrograph. This should be removed.

mbucknell · 2017-07-31T13:57:42Z

floodviz/thumbnail/thumbnail.js

@@ -0,0 +1,90 @@
+// Dependency Import


Could you start with a brief comment describing what this script does

mbucknell · 2017-07-31T14:00:55Z

floodviz/thumbnail/thumbnail.js

+		'./floodviz/static/bower_components/d3/d3.js',
+		'./floodviz/static/bower_components/proj4/dist/proj4.js',
+		'./floodviz/thumbnail/hydro_thumbnail.js',
+		'./floodviz/thumbnail/map_thumbnail.js'


Please remove the map_thumbnail.js

mbucknell · 2017-07-31T14:09:22Z

floodviz/thumbnail/thumbnail.js

+				// Refactor Later. I'm assuming this will change with references.json
+			}
+		);
+		convert(hydro_figure,window,'floodviz/static/css/hydrograph.css','floodviz/thumbnail/thumbnail_hydro.png');


Should be spaces after ','

mbucknell · 2017-07-31T14:10:06Z

floodviz/thumbnail/thumbnail.js

+
+// Wrapper around svg2png that injects custom css to inline svg before conversion
+function convert(figure, window, css_path, filename) {
+	figure.init(undefined);


It's better style to put this after the variable declarations. You can write this as figure.init().

mbucknell · 2017-07-31T14:13:41Z

floodviz/views.py

@@ -91,4 +96,9 @@ def _map_helper():
            ]
        }
    })
+
+    if thumbnail:


Please remove. Let's stick to just the hydrograph.

mbucknell · 2017-07-31T14:14:23Z

package.json

@@ -18,5 +18,9 @@
  "homepage": "https://github.com/USGS-VIZLAB/active-flood-viz#readme",
  "devDependencies": {
    "bower": "1.8.0"
+  },
+  "dependencies": {
+    "jsdom": "11.1.0",


These are actually dev dependencies since they are only used in the build process.

ede0m added 15 commits July 19, 2017 12:54

merge upstream

1692bee

a start. IMPLEMENT

c2d12c7

scrapy stuff here atm, but the svg string export is working. Will nee…

ffd2fed

…d to refactor the figure modules a bit to clean up this code

merge upstream

2f6438d

getting further .. I guess. Tried using svg2png and canvg and neither…

9375729

… have worked..

moving to use phantomjs

6df16df

Somthing working! Reverted back to jsdom

bef9932

use jsdom to pass window to hydromodule thumbnail

5ea65ec

rivers wasnt worked on. Pesky rivers

8a05655

hydrograph.html not relevant

1064c9f

implementing map thumbnail ability

a40807a

Merge branch 'master' of https://github.com/USGS-VIZLAB/active-flood-viz

4ab507d

into thumbnail_FV_34

added support for map png

b1bee7a

directory spelling

8c20cbb

directory names in path

bf9ea85

skaymen reviewed Jul 26, 2017

View reviewed changes

ede0m added 2 commits July 27, 2017 13:51

moved conversion logic to function

bcaa845

added argument parser to avoid overlapping png images

26d0ff9

ede0m added 2 commits July 27, 2017 15:19

exit if invalid arguments

9daea30

added svg2png to package.json depenecies

b2dc43f

mbucknell requested changes Jul 27, 2017

View reviewed changes

ede0m added 5 commits July 28, 2017 09:43

config var for writing flat files

62e7091

Merge branch 'master' into thumbnail_FV_34

a14baeb

added argument parser and inline style injecttion for external css

51456c7

Merge branch 'thumbnail_FV_34' of https://github.com/ede0m/active-flo…

0ae52f9

…od-viz into thumbnail_FV_34

removed map code. added error handling for external css bad file path

1d76943

mbucknell requested changes Jul 31, 2017

View reviewed changes

removed some unnecessary code

d1574db

spaces

7e7b94e

mbucknell approved these changes Jul 31, 2017

View reviewed changes

mbucknell merged commit eed3755 into USGS-VIZLAB:master Jul 31, 2017

ede0m mentioned this pull request Jul 31, 2017

Refac thumbnail fv 73 #93

Merged

ede0m deleted the thumbnail_FV_34 branch August 1, 2017 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thumbnail fv_34 with JSDOM #79

Thumbnail fv_34 with JSDOM #79

ede0m commented Jul 25, 2017 •

edited

Loading

skaymen left a comment •

edited

Loading

skaymen Jul 26, 2017

skaymen Jul 26, 2017

skaymen Jul 26, 2017

skaymen Jul 26, 2017

ede0m commented Jul 26, 2017 •

edited

Loading

ede0m commented Jul 27, 2017

mbucknell left a comment

mbucknell Jul 27, 2017

ede0m Jul 27, 2017

mbucknell Jul 27, 2017

ede0m Jul 27, 2017

mbucknell Jul 27, 2017

ede0m Jul 28, 2017 •

edited

Loading

ede0m Jul 28, 2017 •

edited

Loading

mbucknell Jul 28, 2017

mbucknell Jul 27, 2017

mbucknell Jul 27, 2017

mbucknell Jul 27, 2017

mbucknell Jul 27, 2017

ede0m Jul 27, 2017

mbucknell Jul 27, 2017

mbucknell left a comment

mbucknell Jul 31, 2017

mbucknell Jul 31, 2017

mbucknell Jul 31, 2017

mbucknell Jul 31, 2017

mbucknell Jul 31, 2017

mbucknell Jul 31, 2017

mbucknell Jul 31, 2017

mbucknell Jul 31, 2017

Thumbnail fv_34 with JSDOM #79

Thumbnail fv_34 with JSDOM #79

Conversation

ede0m commented Jul 25, 2017 • edited Loading

skaymen left a comment • edited Loading

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

ede0m commented Jul 26, 2017 • edited Loading

ede0m commented Jul 27, 2017

mbucknell 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

ede0m Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

ede0m Jul 28, 2017 • edited Loading

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

mbucknell 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

ede0m commented Jul 25, 2017 •

edited

Loading

skaymen left a comment •

edited

Loading

ede0m commented Jul 26, 2017 •

edited

Loading

ede0m Jul 28, 2017 •

edited

Loading

ede0m Jul 28, 2017 •

edited

Loading