Alternate strategy for brushing in composite charts

[kum-deepak]

It implements an approach where in composite charts brush is applied only on the parent container. This potentially makes brushing more consistent for these charts.

[gordonwoodhull]

I was about to just go ahead and merge this because it's frustrating trying to debug things like the range series recursion bomb #1424 (and this fixes that!)

But it really changes the meaning of a composite chart, since currently it's not actually a requirement that the composite chart and its children have the same dimension, or that the composite chart have a dimension at all (!) Without a dimension, filtering can't happen.

Also, applying the same filter to all the children is inefficient if they have the same dimension. This is what I attempted to address in the sync-filter branch, also related to #682. If charts have a mechanism where they can hear what their filter should be, without also trying to apply it to the dimension, that will be much more robust.

It's probably good to add the restriction that a composite and all its children share the same dimension, but I think we should document it and add examples that attempt to exercise the related issues, which are: #390 #479 #677 #706 #878

IOW let's do this, but let's do it methodically, with examples and tests.

[kum-deepak]

When I did this, I had much simpler goals in mind. I realized that all the children will have the same xAxis and x scales - which I think is the case.

Then I noticed that the previous code was causing Brush to be created in each of the children - this seemed a path full of land mines. To complicate things further, on brush events (function _chart.fadeDeselectedArea) - the brush in the children were replaced by brush in the parent.

So, what I targeted to achieve was the following:

• Instantiate brush only in the parent - D3 brushes do not care about any data being there - it just manages the physical brush rendering, state (in physical coordinates) and events.
• Instruct children not to instantiate brushes.
• In brush events - get the screen coordinates and inverse map it to x scale; which I thought can be done as all charts share the scale.
• Then pass it on to each of the charts to filter themselves.
• I then realized that the line chart has some behavior change based on brushing being on. This led me to create concept of parentBrushOn.

I think above makes logical sense. I, so far, did not consider dimensions.

My general approach - dc currently does not impose restrictions in many cases. This may make certain non conventional use cases work. As library maintainers we should work towards keeping the code maintainable. I will prefer having a stable feature that works consistently over supporting some non trivial cases.

A similar case I have found regarding range/focus pairs - I could not think of any logical case of keeping domains in range/focus pairs to be different. Currently dc does neither enforce neither rely on that. This I had kept for post version 3.0 for our discussions first. Just an example 😄

Back to my philosophical track - this library should allow mere mortals to create beautiful linked visualizations. Advanced users will figure out their way by modifying dc.js code or directly coding to d3.

I remember my days - I tried to use d3 directly, got thoroughly lost. Then I switched to dc and in few hours - thanks to annotated source of stocks example - my first prototype was ready. In few days my application was ready for prime time.

[kum-deepak]

In simple words what I mean to say, that if I made any change that requires the parent to have anything (e.g., dimension) that was needed earlier. These are non intentional and should be possible to avoid.

[gordonwoodhull]

Good points. Let's return to this soon. I think it will help a lot.

[kum-deepak]

I have been thinking more about it. The following are my findings:

• Usually all the children will share the dimension of the parent.
• One logical case I could think about when a Scatter plot is added as a child, it need to have a different dimension (these need an [x,y] type dimensions).

Do you see any other cases when a child will not share a parents dimension?

As a starting point I will add test cases covering both the variants. I will also see if adding a example of the 2nd variant will help (1st variant example already exists).

[kum-deepak]

Once the test cases and example is in place, I will try optimizing so that each dimension is filtered only once. During that process it needs to be ensured that fadeDeselected works correctly for children.

[gordonwoodhull]

People do all sorts of crazy things with dc.js, so yes, I have seen examples where people have done composites with different, but compatible, dimensions for the child charts.

I have also thought about this further, and nothing about your changes really forces the dimension to be the same. What it really means is that the filter type and filter coordinates have to be the same.

So this may make it difficult to compose a scatter plot with a line or bar chart, since a scatter plot expects a two dimensional range instead of 1D. But I don't think it's worse than it was.

If you're up for the challenge, I think the best way to go about this would be to create a bunch of examples. Line vs scatter, scatter vs bar, bar vs line, bubble vs line, all three, all four, etc. We already have the currently-failing range series example. Like you say, part of it is verifying that fadeDeselected works, also that brushing has the expected effect.

Try to cover the cases mentioned in the issues linked above (although not all of them may have provided enough info to repro).

See which combinations fail without your changes, which ones are fixed by them, which ones still fail.

See if anything can be done, but don't worry about it too much.

Mark the failing examples - they could go in separate directories, like the transitions examples which have a lot of issues.

We can try not to break things which used to work, and we can try to get more things working. It will never be perfect and that's OK!

[kum-deepak]

I have spent further time on this. I was definitely intrigued that how the brushing worked when a scatter plot was combined with a bar/line chart. BTW, this works in this version of code as well.

Then I realized the constructor of 2D Range Filter also accepts a simple array of x min/max and constructs a filter with y values set to -Infinity to +Infinity.

The tests cover such a case. I am planning to yank that and create an example out of that.

In summary I have not found a case where something worked earlier which should fail now.

Another thing that I have noticed that there are valid cases of linking multiple charts to a single dimension. Some cases work, but in cases, in particular, involving filters and highlighting - dc may no longer be friendly. I am unable to think of a solution without refactoring concept of filters. I am actively avoiding that as of now. 😄

I will keep updating as I go along.

[kum-deepak]

#681, #682 - currently not attempting to fix - unable to think of a solution without refactoring concept of filters.

#479 - range series chart http://localhost:8888/web/examples/range-series.html works without hacks. I have taken liberty of adding a small yAxisPadding. The older version I have kept as http://localhost:8888/web/examples/range-series-legacy.html (to be removed during final merge).

#390 - http://localhost:8888/web/examples/range-series.html working without hacks verifies this as well.

#677, #706 - These are related but not related. I will open one more PR once this one is merged.

#878 - I will add an example, it should work now.

In this process I also realized a minor issue - logged as #1431 (non priority)

[kum-deepak]

• Add example for Brush not filtering when using composite chart #878
• Example for scatter plot (2D Filterable) combined with some 1D filterable charts within same Composite Chart

[kum-deepak]

While enabling brushing in http://localhost:8888/web/examples/scatter-series.html I encountered a small bug in scatterPlot. I have committed that as a separate commit - 976e8f6

@gordonwoodhull please review that commit and if you suggest, I can submit that as a separate PR.

[kum-deepak]

My final commit for this round, awaiting your review 😄

Once you have confirmed which issues it solves fully/partially, I will make entries in the change log.

[gordonwoodhull]

Hurray! This makes things so much simpler. I love a PR that fixes things by removing code.

I am going ahead and writing the release notes and putting this in release 3.0.2, making a couple of minor changes. That's easier than trying to write it up so that you can write it up again. 😉

I think that #706 as described (composite chart should change the filters on the children) was already implemented, but it didn't work right. So I consider it fixed by this PR. Do you disagree? We can reopen if necessary.

The "composite brushing" example is great, and well explained. It's a little more specific, so I'm giving it the longer title "composite brush multi dim".

What a relief to see all these longstanding issues fixed. Thanks a million @kum-deepak!

Now let's put it out there and see if there's any blowback!

[kum-deepak]

Thanks @gordonwoodhull, let us wait for reports from users 😃

The code removal was possible because of the two PRs that preceded around brushing and zooming. I am also finally happy with the outcome. 😇

I did not expect the legacy version of range series to continue as I do not see it fulfilling any purpose now.

[kum-deepak]

For #677 and #706 - my interpretation was that when a filter is applied on the composite chart, the same filter should get applied to all the children (currently it will need to be applied on each of the children in a loop). With the recent changes, my guess, it should be possible to do. Will wait for your opinion before jumping to this one.

[gordonwoodhull]

Yes, I meant to delete the legacy range series example. We can do that next version.

Got it, #677 and #706 are the programmatic version of this - thanks for explaining. Since this is most of what applyBrushSelection does, it shouldn't be hard to move that code into filter override, and create filterAll / replaceFilter overrides if needed.

[kum-deepak]

Please see #1435.