Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Data.KeyFunction #2870

Merged
merged 29 commits into from
Oct 19, 2015
Merged

Conversation

softwords
Copy link

Resubmitted against objectConstancy branch - previously #2833
also see issue #2512
This pull request

adds the key function to Dataset
uses this key in Drawer when binding data to the selection
provides 2 predefined keys:
-- NoConstancy - which will return a new value each time it is called, therefore ensuring that there is never object constancy; that is, all elements in the selection are removed and recreated
-- useIndex - returns the index of the datum n the array, which is the default behaviour of d3 anyway

This PR provides a necessary foundation for developing animations in plottable that respect "object constancy" - proving visual representation of elements moving, entering or exiting the datasets being plotted.

@bluong
Copy link
Contributor

bluong commented Oct 12, 2015

Can you edit your pull request description to reference the other pull request and vice versa? After that, we can close the other pull request, and we can continue discussion here.

@bluong
Copy link
Contributor

bluong commented Oct 12, 2015

Feel free to copy paste the original pull request description here as well. Specify what exactly this pull request brings to the table (if it's nothing at all from user-perspective, then mention this).

@bluong bluong self-assigned this Oct 12, 2015
@bluong
Copy link
Contributor

bluong commented Oct 12, 2015

I just pushed in the changes for our Gruntfile. Can you check to see if this helps?

@bluong bluong self-assigned this Oct 15, 2015
@bluong
Copy link
Contributor

bluong commented Oct 15, 2015

Whoops, it seems that I missed something. @softwords can you look into my comment? After you follow up, feel free to proceed with your next pull request.

@bluong bluong added the +1 label Oct 15, 2015
@bluong bluong assigned crmorford and unassigned bluong Oct 15, 2015
@crmorford
Copy link
Contributor

Hey @softwords, I just wanna say sorry for the holdup, I was pretty busy at the end of last week. I'm going to take a look at this now :)

@crmorford
Copy link
Contributor

Testing Fiddles
No action required by softwords

Plot Types:
Horizontal Bar (normal, stacked, clustered)
Vertical Bar (normal, stacked, clustered)
Scatter
Line
Area
Pie

Axis Types:
Linear
Log
Time
Category

Data stuff:
2 datasets with no overlap
2 datasets with all overlap
dataset doesn't have key identified
change key function from category1 --> category2
key function uses combination of 2 categories?
change fill function based on index http://jsfiddle.net/oaskqta0/13/

edit: just kidding! apparently this is mostly a regression pass.
not deleting this list, because I'll use it later on for following PRs.

@crmorford
Copy link
Contributor

Hi @softwords,

In this fiddle, I'm using the rankcolor function that you had written (with some tweaks), and there's something I'm confused about.

rankColor uses the index to color bars. It seems to work correctly when there is no object constancy or when it's joined on the index, but I'm seeing something I don't expect when object constancy is enabled.

It starts out looking good:
screen shot 2015-10-19 at 11 42 20 am

But when I press the "2010" button, the blues and purples get mixed up:
screen shot 2015-10-19 at 11 42 37 am

I would expect the fill function, rankColor, to just be based on the index, but it seems like something else is going on. Can you explain that?

@crmorford
Copy link
Contributor

adding on to last comment:
on plottable develop the same fiddle (minus some code) uses the rankColor function the way i'd expect when you click 2006/2010/2014 buttons -- the colors always go in rainbow order
http://jsfiddle.net/oaskqta0/14/

on this branch:
uncommenting the ds.keyFunction() lines, and clicking the 2006/2010/2014 buttons causes different coloring than expected -- not in rainbow order
http://jsfiddle.net/oaskqta0/15/

I don't want to QE +1 this until I understand what's happening, but this is the only thing i've found so far. regression pass looks good.

@crmorford crmorford assigned bluong and unassigned crmorford Oct 19, 2015
@bluong
Copy link
Contributor

bluong commented Oct 19, 2015

Looked into your JSFiddle @crmorford and I found why there is an inconsistency (which seems to me that we should ship with the index constancy which I believe keeps the default behavior). It has to do with how D3 does its enter/exit selections in accordance with the data.

Unless I'm mistaken, let's say I have an array [o1, o2, o3]. Now let's say I change the data to [c1, c2, c3], however let's say that key(o1) === key(c2), then I believe D3 will actually count c2 to be existing where o1 used to exist. Thus, the input indices are modified.

@bluong bluong assigned crmorford and unassigned bluong Oct 19, 2015
@bluong
Copy link
Contributor

bluong commented Oct 19, 2015

The realization that the "same" datum maintains their index across changing data is something that caught us off guard, but it makes sense after a bunch of discussion. We're going to need some time to think about think specific realization, but we should come up with an answer soon.

@crmorford
Copy link
Contributor

Ok, this looks good for now. I think it'll be far easier to test intensively when the next PR comes in, and I don't see it causing any problems with existing use cases.

@crmorford crmorford added QE +1 and removed QE -1 labels Oct 19, 2015
@bluong bluong assigned bluong and unassigned crmorford Oct 19, 2015
bluong added a commit that referenced this pull request Oct 19, 2015
@bluong bluong merged commit ce7a905 into palantir:objectConstancy Oct 19, 2015
@bluong
Copy link
Contributor

bluong commented Oct 19, 2015

@softwords We are awaiting your next pull request!

@softwords
Copy link
Author

@bluong I was just replying to @crmorford when the merge came through, so I am overtaken by the speed of events!
Ok - I think now, for your risk management point of view, I should put up everything that I now have on this. You can see the strategy (and code!) and decide sooner rather than later on architecture issues, (or indeed if you want to proceed) and gauge the amount of effort involved in getting it over the line.
Take a look at it, and we can then think about whether you want to try to push it all through in one big PR, or try to break it up into smaller bits.
stay tuned!

@softwords
Copy link
Author

hello @crmorford,
-- I just wanna say sorry
no apology required! :)
Looking into your fiddle, notice that the x co-ordinate of each bar is taken from the R property in the data, rather than from the index.
If your color function becomes

    rankColor = function (d) {
        if (d.R == 1) return "#FF0000";
        if (d.R == 2) return "#FFA500";
        if (d.R <= 4) return "#FFFF00";
        if (d.R <= 8) return "#00FF00";
        if (d.R <= 16) return "#0000FF";
        return "#FF00FF";
    }

then it works as you would expect.
But, this data is sorted in order of R anyway, ie, you would think that d.R - 1 === i
Using this color function

    rankColor = function (d, i) {
        if (d.R - 1 === i) return "#FF0000";
        return "#FF00FF";
    }

demonstrates this is not so when constancy is on.
So @bluong you are right that the issue is to do with the index.
Currently in plottable's animators the attributes are applied to the entire selection - which is in one-to-one correspondence with the incoming data. So in this circumstance you can assume that the index i on the attribute function is the index into the data array. But - when the Animator can work with the enter, update and exit selections created by selection.bind() then the parameter i is passed the index into the targeted selection, which will in general not be the same as the index into the original data.

@bluong
Copy link
Contributor

bluong commented Oct 20, 2015

@softwords , I am mainly curious in your approach regarding the Animators. In earlier pull requests, you seem to reference an Animators.Bar, etc. Can you elaborate on this? If you have a pull request concerning this, I'd be happy to look at this as well.

@softwords
Copy link
Author

@bluong @crmorford I have been putting together these notes explaining what is in my next PR from branch 'DrawingTarget'. (within 24 hours)

DrawingTarget

A key change in this PR is to expose the D3 "enter / update / exit" pattern to the animators.
This introduces the type DrawingTarget, which has properties enter exit update and merge corresponding to these selections. merge is the combination of update and enter - in d3, once the enter().append() method is called on the bound data, that bound data is automatically updated to include the entering elements. merge is therefore in one-to-one correspondence with the incoming data.
The DrawingTarget is created by Drawer in _bindSelectionData. It is then passed to the animators, by extending the IAnimator interface:

  animate(selection: d3.Selection<any>|d3.Transition<any>, attrToAppliedProjector: AttributeToAppliedProjector,
    drawingTarget?: Drawers.DrawingTarget, drawer?: Drawer): d3.Selection<any> | d3.Transition<any>;

Note that _bindSelectionData no longer calls .remove() on the exit selection - otherwise you still could not do anything with it! remove()ing the exit selection is now a responsibility of the animator; see below.

Initializer

Usually, in the d3 "enter update exit" pattern, there is some initialization of attributes when the enter() elements are created; e.g.:

    selection.enter()
      .append(_svgElement)
      .attr(....)

initializer is a new property on Drawer that takes a function ()=> AttributeToProjector. Drawer also gets the method appliedInitializer(): AttributeToAppliedProjector which is the result of the initializer function converted to an AttributeToAppliedProjector.
This is applied to the enter() elements to initialize their state.
The initializer is passed by the plot in its _createDrawer method, so the initializer may have access to the internals of the calling plot. In fact, a number of plots already have the code needed for the initializer - it's precisely the code in the _animateOnNextRender block. So this refactor does away with the pesky _animateOnNextRender logic, and makes the initialization of new elements more "mainstream" d3.

Responsibilities of Animators

Animators now get a lot of opportunities to work in detail with the "enter update exit" pattern, and in particular, to apply transitions to the enter and exit selections. Animators have two responsibilities:
-- remove() the exit selection.
-- update the DrawingTarget when transitions are applied.
For example, if the Drawer derives a transition from drawingTarget.enter, it should update drawingTarget.enter with that transition:

drawingTarget.enter = this.getTransition(drawingTarget.enter, 1000);

The point of this is that a subsequent transition derived from drawingTarget.enter will be a "subtransition", and (if you are careful not to interfere) d3 will handle scheduling this subtransition to chain from the first (see here). This is one way to create more complex staged animations.

Easing Functions

This PR introduces some custom easing functions:
atStart - everything happens immediately at the start of the transition. ie this creates a "wait" after applying the attributes.
atEnd - everything happens in one go at the end of the transition - ie this creates a "wait" before applying the attributes.
squEase - this applies the transition squeezed into a subinterval [a,b] where 0 <= a <= b <= 1. The actual easing to apply in that interval is passed as an argument.
These are designed to help animators schedule animations between the enter update and exit selections - each can have a transition of the same duration, but the actual timing of activity will be determined by the easing function.

Animator Hierarchy

There are a number of new animator classes:
Base - basically equivalent to the current Easing, but with various protected helper methods available to subclasses.
Attr - apply a set of attributes to enter coming in - and to exit going out.
Opacity - derived from Attr, fades incoming/outgoing in and out.
Callback - gets the actual animate function as a property. This allows the javascript client full access to the "enter update exit" selections. I think this is in keeping with the philosophy that plottable will always provide a way to get directly to d3 for occasions when you need something not provided in higher level code.
Reset - applies the Drawer's appliedInitializer to the entire merge selection. (this is a key reason for Drawer passing itself to the animator).
Bar - a staged animation specific to bar charts, that exits, then moves, then enters elements in turn. Demonstrates multiple transitions, custom easing functions, and the various helper functions in Animators.Base.

Legacy behaviour

Current animation resets all elements to a "start" state, and animates them back to their final state. To get this to happen in the new architecture, the Drawer's appliedIinitializer has to be applied to all elements, not just the enter()ing elements. Two ways to do this:
-- use the noConstancy key. With noConstancy, every element is always in enter, so it gets initialised.
-- use the Reset animator.

Quicktests

See the animation.html in the quicktests/overlay folder. Click on any of these animations to toggle between two datasets. The data items in these datasets have a name property, a new utility function makeRandomNamedData() generates names with some degree of overlap - so you get enter update and exit selections to observe. The dropdown bar lets you choose between key functions - useProperty("name") (ie constancy), noConstancy, and useIndex.

function.prototype.bind

To maintain this as the context of the hosting plot, initializer functions use function.prototype.bind, which it turns out is not supported by phantomjs versions < 2.0.0. I have added a polyfill into coverage.html to get around this.

//# sourceURL

The new quicktests use the annotation
//# sourceURL=<url>
as the last line of the file. When chrome loads the file dynamically, the sourceURL annotation causes it to appear in the Source page list of files, which makes it much easier to examine and debug. I found it's better placed at the end rather than the start, because chrome removes the line, and thereafter seems to be one line out when you are stepping through the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants