basic brushing and zooming feature for line chart and point chart #851

cnwangjie · 2018-05-04T01:50:07Z

This basically implements the brushing and zooming feature like mg-line-brushing. It may be not ready to be merged into master branch. But I hope to share some problems I found and to get some suggestions.

Some difference with the addons

I take a more independent approach to this feature and decompose it into some functions for easier to control. I add two options to allow the user to use this feature. If not set will not have any effect on the original function.
Compared with addons, I will add the brushing function when the chart's initialization function be called and directly use the descriptor to redraw the chart for avoiding repeated call hooks.

Some problems

For the line chart, we need not to filter the data out of range in order to ensure the integrity of the line.
But for the point chart, we should drop those data out of range, if not there will be some points drawn outside the chart because of the different ways of drawing. I tried to filter them in the mainPlot method in point.js but the axis will still be drawn. So I filter the data when the chart type is point. I don’t know if this is appropriate?

Because different types of charts are drawn in different ways, some changes need to be made for them. Do I need to support this function for other charts except line and point?

I largely borrowed code of the addons, so that some operations I'm not sure whether it has the right effect. Such as convert mouse selection range to data domain and set the range of selection graphic.

Here is my effect on manual testing of some data.

Now I only added brush and zoom_target. This is enough to restrict brushing axis and add an overview plot. Should I add more options or is there any better option?

For setting a zooming target chart, I don't know how to do it better. The way I am now taking is to set an args as a variable and set it to another option. Like following way. Are there any better ways?

const main = {
  title: "UFO Sightings",
  description: "Yearly UFO sightings from 1945 to 2010.",
  data: JSON.parse(document.querySelector('.data textarea').value),
  markers: [{'year': 1964, 'label': '"The Creeping Terror" released'}],
  width: 400,
  height: 250,
  target: ".result",
  x_accessor: "year",
  y_accessor: "sightings",
  brush: 'x',
}
MG.data_graphic(main);
MG.data_graphic({
  data: JSON.parse(document.querySelector('.data textarea').value),
  width: 400,
  height: 150,
  target: ".overview",
  x_accessor: "year",
  y_accessor: "sightings",
  brush: 'x',
  zoom_target: main,
  x_axis: false,
  y_axis: false,
  show_rollover_text: false,
});

wlach · 2018-05-04T19:05:51Z

@cnwangjie great start! a few things:

I would not worry about the overview/zoom target chart for now. impressive that you got that working so fast, but let's save that feature for a followup PR. :)
No, I don't think we need this API for anything other than line and point. It doesn't make sense to zoom in a histogram chart, for example.
I think filtering the data to only show stuff that should be visible is fine. Would need to look at your implementation to be sure (will do after writing this).
I noticed a few bugs in the implementation:

zooming does not seem to work for point charts (chart_type: point) for me. I used the existing interactive example used by gulp serve, just with that extra parameter
if I move the cursor outside the graph, then move it back again, zooming gives odd results. example:

5. We'll want to add the relevant files to the examples for testing. e.g. for the interactive example:

diff --git a/src/index.html b/src/index.html
index 5395f20..b536f86 100644
--- a/src/index.html
+++ b/src/index.html
@@ -22,6 +22,8 @@
     <script src='js/MG.js'></script>
     <script src='js/misc/utility.js'></script>
     <script src='js/common/data_graphic.js'></script>
+    <script src='js/common/brush.js'></script>
+    <script src='js/common/zoom.js'></script>
     <script src='js/common/hooks.js'></script>
     <script src='js/common/register.js'></script>
     <script src='js/common/bootstrap_tooltip_popover.js'></script>

wlach

Just some minor coding style comments based on a quick once over. I may request other changes later. :)

wlach · 2018-05-04T19:11:02Z

src/js/common/brush.js

@@ -0,0 +1,95 @@
+function mg_create_brushing_pattern(args, range) {
+  var extentRect;


Please use const and the ternary operator here:

const extentRect = (d3.select(args.target).select('.mg-extent').size()) ? d3.select(args.target).select('.mg-extent') : d3.select(args.target) .select('.mg-rollover-rect, .mg-voronoi') .insert('g', '*') .classed('mg-brush', true) .append('rect') .classed('mg-extent', true);

In general we should try to use let or const for new code (http://airbnb.io/javascript/#references--prefer-const)

wlach · 2018-05-04T19:11:39Z

src/js/common/brush.js

+  var mouseDown = false;
+  var origin = [];
+  rollover.classed('mg-brush-container', true);
+  rollover.on('mousedown', function() {


New functions should be defined using the arrow syntax:

http://airbnb.io/javascript/#arrows--use-them

wlach · 2018-05-04T19:16:56Z

src/js/common/zoom.js

+    var xScale = args.scales.X;
+    domain.x = [];
+    domain.x[0] = +xScale.invert(range.x[0]);
+    domain.x[1] = +xScale.invert(range.x[1]);


Better to define the array like this:

domain.x = range.x.map(v => +xScale.invert(v));

cnwangjie · 2018-05-05T16:18:26Z

@wlach Thank you for your review. I have updated this PR, following are changes I made.

fixes some obvious bugs

For the bug you say:

if I move the cursor outside the graph, then move it back again, zooming gives odd results.

I think it is because the selection range is too small (no any data point in this range). When the mouse leaves the chart and up, there will not be mouseup event when you return to the chart again. So it's possible to make a click and select a too small range. I‘ve solved it by changing event listener target to document. Then the events can be triggered on the whole page. In this way, I think the interactive experience will be a little better. However, if the user intentionally select a very small range, it still causes this phenomenon. I haven’t found a good way to avoid it yet.

But for zooming does not work for point charts, it can work on me. Could you try it again? If it still doesn't work, I will make some changes for it.

I added two attributes zoom_x & zoom_y to args.processed, but forgot to delete it when it is not needed.

I've added these two files to the src/index.html, I did it but forgot to stage this file before.

I changed the two files I added to ES6 style

You want to use the keywords let & const and arrow function. I think it is great, but it is a ES6 feature. I think a good project should keep the code style uniform. But this project was previously written in ES5 and previous styles. So I've avoided using any ES6 features before. Since you want to use new features, it is great for a modern JS project, so I tend to use more modern styles completely.

I also hope that this project can be more modular. And I'm happy to make changes for this for a long time.

Can you look it again? If there are no other problems and you think it is ready to be merged, I would squash these commits.

wlach · 2018-05-08T21:11:01Z

@cnwangjie hmm, so I downloaded your latest changes and it looks like things are working better with point charts, but still not quite working. I can drag a selection but nothing seems to actually select. Here's the js I have:

//modify away!
MG.data_graphic({
  title: "UFO Sightings",
  description: "Yearly UFO sightings from 1945 to 2010.",
  data: JSON.parse(document.querySelector('.data textarea').value),
  markers: [{'year': 1964, 'label': '"The Creeping Terror" released'}],
  width: 400,
  height: 250,
  target: ".result",
  x_accessor: "year",
  y_accessor: "sightings",
  xax_count: 3,
  chart_zoom: "xy",
  chart_type: "point"
});

Here's the error I'm seeing in the console:

The code itself is looking pretty good! Most of the code is indeed written in ES5 style, but we're gradually moving things over. I'm hoping we'll have the process complete by end of summer or so.

wlach · 2018-05-08T21:14:03Z

src/js/common/zoom.js

+  }
+
+  return d => {
+    const x_in_range = ('x' in range) ? is_data_in_range(d[args.x_accessor], range.x) : true;


This could be restated more simply as:

const x_in_range = !('x' in range) || is_data_in_range(d[args.x_accessor], range.x);

Same with the statement just below.

cnwangjie · 2018-05-10T14:45:07Z

@wlach I spent some time finding the reasons for this case.

The direct reason is the type of "year" is string, so args.x_axis_type is 'categorical' and it will use d3.scaleBand to make scale. But the band scales without invert method so I can not calculate the range of data based on the selection range by directly using invert method that d3 provided.

I can infer the range of the data that needs to be displayed for this situation. But I'm not sure if it's appropriate to add brushing function to this type (categorical) chart.

If we convert this data to number another problem (same as #822) will be displayed. I wrote this problem in detail under that issue. But I'm not sure what the auhtor intended, so I just put forward some of my ideas and did not directly modify it. I hope to discuss this with you and other members.

wlach · 2018-05-10T20:35:54Z

@cnwangjie oh right! that would explain it. for a first pass, it might just make sense to disable/ignore zooming on this type of chart (categorical). @hamilton -- do you have any opinions here?

hamilton · 2018-05-10T20:38:03Z

I concur - let's disable / ignore zooming for categorical charts for now.

wlach · 2018-05-16T13:44:04Z

src/js/common/brush.js

+    if (mouseDown) {
+      isDragging = true;
+      rollover.classed('mg-brushing', true);
+      const range = calculateSelectionRange();


No need to create this variable, just pass the value of calculateSelectionRange() directly to the function below.

wlach · 2018-05-16T13:44:42Z

src/js/common/brush.js

+      MG.zoom_to_raw_range(args);
+    }
+  });
+


Remove this newline

wlach · 2018-05-16T13:45:28Z

src/js/common/zoom.js

+
+const zoom_to_raw_range = args => {
+  if (!('raw_domain' in args)) return;
+  const range = args.raw_domain;


Again, no need for a temporary here.

wlach · 2018-05-16T13:46:39Z

src/js/common/zoom.js

+const zoom_to_data_domain = (args, range) => {
+  const raw_data = args.raw_data || args.data;
+  if (!('raw_data' in args)) {
+    args.raw_domain = {


I would probably put raw_domain and raw_data under args.processed, since they don't represent user input.

I think so, but there are many variables under args.

wlach · 2018-05-16T13:52:01Z

src/js/common/zoom.js

+      args.data = raw_data.filter(filter_in_range_data(args, range));
+    }
+  }
+  if ('x' in range) args.processed.zoom_x = range.x;


Maybe try this approach instead:

['x', 'y'].forEach(dim => { if (dim in range) { args.processed[`zoom_${dim}`] = range[dim]; } else { delete args.processed[`zoom_${dim}`]; } });

wlach · 2018-05-16T13:52:44Z

src/js/common/zoom.js

+    const yScale = args.scales.Y;
+    domain.y = range.y.map(v => +yScale.invert(v)).reverse();
+  }
+  return domain;


Could probably do something similar to what I suggested just above with a forEach loop here.

If you need to reduce the number of lines of code this function can be written as follows. And just only avoid one repeat.

const convert_range_to_domain = (args, range) => ['x', 'y'].reduce((domain, dim) => { if (!(dim in range)) return domain; domain[dim] = range[dim].map(v => +args.scales[dim.toUpperCase()].invert(v)); if (dim === 'y') domain[dim].reverse(); return domain; }, {});

Also it is probably to change

return d => { const x_in_range = !('x' in range) || is_data_in_range(d[args.x_accessor], range.x); const y_in_range = !('y' in range) || is_data_in_range(d[args.y_accessor], range.y); return x_in_range && y_in_range; }

to

return d => ['x', 'y'].every(dim => !(dim in range) || is_data_in_range(d[args[`${dim}_accessor`]], range[dim]))

Also some others.
But does this make the code a bit difficult to understand? 😕
I'm not sure if this is necessary?

@cnwangjie I think it's pretty understandable, at least to someone who knows what they're looking for. In general I prefer this kind of "denser" style of code-- it requires a bit more functional programming knowledge to understand, but it's much more concise.

If you think a particular section is unclear, it never hurts to put in a comment like this:

// Converts a selection range into a set of coordinates that we can use to zoom // the chart to a particular region const convert_range_to_domain = (args, range) => ...

wlach · 2018-05-16T13:53:56Z

src/js/common/brush.js

+
+const add_brush_function = args => {
+  if (!args.zoom_target) args.zoom_target = args;
+  const zoom_target = args.zoom_target;


I don't think you need this temporary variable.

cnwangjie · 2018-05-17T09:25:23Z

@wlach I have modified my code based on your suggestion. And display warning when axis type is 'categorical'. But I have some confusion about the necessity of doing so. I used to think that the code should be easy to understand, so I like to use some temporary variables. And expect the compiler to optimize the performance of the code. But if you think that it should be done, I'm also happy to make some changes to do it. 😂

wlach · 2018-05-17T21:20:57Z

@cnwangjie yes, of course the compiler with optimize the variables. but I think it's still a bit unnecessary to use so many temporaries. This is largely a matter of preference, but I'd encourage you to give it a try for a little while and see how you like it. :) As I mention above, comments can often help explain more difficult sections.

wlach · 2018-05-17T21:29:24Z

@cnwangjie ok, I'm pretty happy with things now! One last thing before we merge, can you update
the examples to remove the reference to the outdated plugin, and replace it with your code:

https://github.com/metricsgraphics/metrics-graphics/blob/master/examples/charts/addons.htm

Probably best to rename the file to "brushing_zooming.htm" and "addons" to "brushing & zooming"

cnwangjie · 2018-05-18T05:45:38Z

@wlach OK. I have updated the examples and supplied comments for some functions, and squashed unnecessary history commits. Besides, I added some simple descriptions for two new options in wiki "List of Options" & "Graphic" pages. I think I can add a link to the example in the wiki after the example pages are updated.

wlach

Looking good! Just a few minor changes requested

wlach · 2018-05-18T20:53:12Z

examples/charts/brushing_zooming.htm

+    }
+    const main = {
+        title: "Overview Plot",
+        description: "This is a simple for creating an overview plot. You can create an overview plot easily by creating another chart with 'zoom_target' option and set it as the object of main chart. Besides, you can hide axis and active point to simplify the overview plot.",


"This is a simple for creating an overview plot. You can create an overview plot easily by creating another chart with 'zoom_target' option and set it as the object of main chart. Besides, you can hide axis and active point to simplify the overview plot."

=>

"This is a simple example of an overview plot. You can create an overview plot by creating another chart with 'zoom_target' option and then setting it as the object of the main chart."

P.S. I didn't realize you left the overview plot functionality in. :) That's ok-- there are a few features I would like to add to that in a followup. We can discuss next week. :)

wlach · 2018-05-18T20:53:59Z

examples/charts/brushing_zooming.htm

+    }
+    MG.data_graphic({
+        title: "Basic Brushing & Zooming",
+        description: "This is a simple for brushing and zooming feature. You can set 'brush' as 'xy', 'x', 'y' to restrict axis.",


"This is a simple for brushing and zooming feature. You can set 'brush' as 'xy', 'x', 'y' to restrict axis." => "This is a simple example of brushing and zooming. You can set 'brush' as 'xy', 'x', 'y' to specify the axis(es) that may be brushed."

wlach · 2018-05-18T20:55:21Z

examples/examples.htm

@@ -100,7 +102,7 @@
            <li><a href='#' id='goto-updating' class='pill'>Updating</a></li>
            <li><a href='#' id='goto-other' class='pill'>Other</a></li>
            <li><a href='#' id='goto-experimental' class='pill'>Experimental</a></li>
-            <li><a href='#' id='goto-addons' class='pill'>Addons</a></li>
+            <li><a href='#' id='goto-brushing_zooming' class='pill'>Brushing & Zooming</a></li>


Could you put this before experimental? Also, you should generally use & for specifying the ampersand in html :)

- change some syntax to ES6 & fix some obvious problems (5.5) - change some format & warning on categorical (5.17) - display warning when axis type is 'categorical' - add some comment - update the example page for this feature and remove outdated addons (5.18)

cnwangjie · 2018-05-19T10:45:36Z

@wlach ok, I've changed.

wlach · 2018-05-19T23:32:17Z

Awesome! It's a holiday weekend here in Canada, I'll do one last review if I have the time, otherwise I will get to this first thing Tuesday.

wlach · 2018-05-23T02:22:02Z

Thanks @cnwangjie. As mentioned there are a few tweaks i would like to make to this implementation (e.g. I would like to persist the selection in the case that we're brushing over an overview chart), but i think this is a good incremental step. Impressive start to the GSOC project!

cnwangjie mentioned this pull request May 4, 2018

Incorporate mg-line-brushing extension directly into metricsgraphics #790

Open

wlach requested changes May 4, 2018

View reviewed changes

wlach reviewed May 8, 2018

View reviewed changes

cnwangjie force-pushed the master branch from 0f4745f to c52d68f Compare May 10, 2018 14:49

wlach reviewed May 16, 2018

View reviewed changes

src/js/common/brush.js Outdated

MG.zoom_to_raw_range(args);

}

});

Copy link

Collaborator

wlach May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this newline

wlach reviewed May 16, 2018

View reviewed changes

cnwangjie force-pushed the master branch 2 times, most recently from 34cef99 to d2e9390 Compare May 18, 2018 05:38

wlach requested changes May 18, 2018

View reviewed changes

cnwangjie force-pushed the master branch from d2e9390 to 8cb6ea6 Compare May 19, 2018 10:33

cnwangjie force-pushed the master branch from 8cb6ea6 to fd92571 Compare May 19, 2018 10:40

wlach approved these changes May 23, 2018

View reviewed changes

One more description tweak

bff1fac

wlach merged commit c4c0ac9 into metricsgraphics:master May 23, 2018

wlach mentioned this pull request May 24, 2018

Brushing & zooming should persist when zooming over an overflow plot #855

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic brushing and zooming feature for line chart and point chart #851

basic brushing and zooming feature for line chart and point chart #851

cnwangjie commented May 4, 2018

wlach commented May 4, 2018

wlach left a comment

wlach May 4, 2018

wlach May 4, 2018

wlach May 4, 2018

cnwangjie commented May 5, 2018 •

edited

Loading

wlach commented May 8, 2018

wlach May 8, 2018

cnwangjie commented May 10, 2018

wlach commented May 10, 2018

hamilton commented May 10, 2018

wlach May 16, 2018

wlach May 16, 2018

wlach May 16, 2018

wlach May 16, 2018

cnwangjie May 17, 2018

wlach May 16, 2018

wlach May 16, 2018

cnwangjie May 17, 2018

wlach May 17, 2018

wlach May 16, 2018

cnwangjie commented May 17, 2018

wlach commented May 17, 2018

wlach commented May 17, 2018

cnwangjie commented May 18, 2018

wlach left a comment

wlach May 18, 2018

wlach May 18, 2018

wlach May 18, 2018

wlach May 18, 2018

cnwangjie commented May 19, 2018

wlach commented May 19, 2018

wlach commented May 23, 2018

		@@ -0,0 +1,95 @@
		function mg_create_brushing_pattern(args, range) {
		var extentRect;

basic brushing and zooming feature for line chart and point chart #851

basic brushing and zooming feature for line chart and point chart #851

Conversation

cnwangjie commented May 4, 2018

Some difference with the addons

Some problems

wlach commented May 4, 2018

wlach 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

cnwangjie commented May 5, 2018 • edited Loading

fixes some obvious bugs

I changed the two files I added to ES6 style

wlach commented May 8, 2018

Choose a reason for hiding this comment

cnwangjie commented May 10, 2018

wlach commented May 10, 2018

hamilton commented May 10, 2018

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

cnwangjie commented May 17, 2018

wlach commented May 17, 2018

wlach commented May 17, 2018

cnwangjie commented May 18, 2018

wlach 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

cnwangjie commented May 19, 2018

wlach commented May 19, 2018

wlach commented May 23, 2018

cnwangjie commented May 5, 2018 •

edited

Loading