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

Enable 'object constancy' on data refresh by supporting a key function in Dataset #2833

Closed
wants to merge 23 commits into from

Conversation

softwords
Copy link

Resubmitting with cleanups to formatting - previously #2801
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, therefire ensuring that there is never object constancy; that is, all elements in the selection are removed and recreated
-- ByIndex - returns the index of the datum n the array, which is the default behaviour of d3 anyway

Also removes the logic on AnimateOnNextRender in ScatterPlot. This logic simulates the destruction and recreation of each svg element to create an animation effect. But, if the NoConstancy key is used, d3 destroys and recreates the svg elements itself, so there is no need for this workaround.
This also resolves issue #2707.

@bluong
Copy link
Contributor

bluong commented Oct 2, 2015

Can you please compile using grunt so we can see the output.

@softwords
Copy link
Author

@bluong I have loaded the plottable.js and plottable.d.ts that I get running test in grunt from the VS Task runner Explorer.
You'll see the issue I mentioned yesterday - that whereever a private declaration has been removed from plottable.d.ts, I have a blank row.

@bluong
Copy link
Contributor

bluong commented Oct 2, 2015

So there is some functionality in our Gruntfile.js that removes the private declarations since it seems that having them is unnecessary. That functionality might be what you're running into.

@softwords
Copy link
Author

Yes I've had a look at that - it's the sed:privateDeclarations task. I've played with it a bit but haven't yet managed to find a way to get the same result in Windows - becuase those regex expressions are looking for \n line-ends I think.
But just on that, I wondered why typescript would even put those private declarations in the .d.ts file - you would think that only the publicly exposed interface matters. There is an explanation here in the typescript github:

Private members are emitted in a declaration file to indicate that a private property with that name exists so that a derived class doesn't accidentally use the name (which would step on the private members).

It certainly looks nicer for the .d.ts file not to have them, and chances of accidently reusing the private variable would seem slim; but by including those private names in the .d.ts you'll get a compile error if you try to override.
So - it may be worth considering leaving them there?

@softwords
Copy link
Author

A lot of things I don't fully understand are happening with the testing stage: as you can see in the travis output phamtomjs is throwing an error. Sometimes I get this locally, but when it runs to completion, I get a number of tasks pending. These tasks are also pending if I run coverage.html in chrome - they all relate to testings for type, or non-numeric values. e.g. currency() - throws exception for non-numeric values.
Other errors are rounding type problems: e.g. pointerinteraction is typical:

calls the onPointerEnter callback (mouse) ‣
AssertionError: was passed correct point (mouse): expected { x: 200, y: 199.33203125 } to deeply equal { x: 200, y: 200 }
    at Context.<anonymous> (tests.js:13773:24)

Again, see the same thing in chrome and phantomjs, although sometimes the fail in phantomjs also reports out of memory:


  10) Plots Bar Plot Horizontal Bar Plot entityNearest() considers only in-view bars:
     Error: Out of memory

chrome reports the error a bit differently:

considers only in-view bars ‣
AssertionError: closest plot data is on-plot data (selection contents): expected <rect fill="#5279c7" width="150" height="103.7037037037037" y="222.22222222222223" x="150"></rect> to equal <rect fill="#5279c7" width="473.6842105263158" height="103.7037037037037" y="222.22222222222223" x="157.89473684210526"></rect>
    at tests.js:109:20
    at Array.forEach (native)
    at assertEntitiesEqual (tests.js:108:29)
    at Object.assertPlotEntitiesEqual (tests.js:115:9)
    at Context.<anonymous> (tests.js:7577:33)

So any guidance you can offer as to what could be going on here would be much appreciated.

@bluong
Copy link
Contributor

bluong commented Oct 2, 2015

One thing that could be happening is inaccuracies due to being on a different OS / different version of a certain browser. In terms of the out of memory error, I am unsure why that would be the case. In terms of the closest plot data errorin and the onPointerEnter callback erroring, it seems to be precision errors?

Can you skip the tests that are failing and then continue with the pull request?

@softwords
Copy link
Author

Looking at these precision errors, running in both Chrome and IE, I have found something interesting:

in this code in TestMethods:

 export function triggerFakeMouseEvent(type: string, target: d3.Selection<void>, relativeX: number, relativeY: number, button = 0) {
    let clientRect = (<Element> target.node()).getBoundingClientRect();
    let xPos = clientRect.left + relativeX;
    let yPos = clientRect.top + relativeY;
    let e = <MouseEvent> document.createEvent("MouseEvents");
    e.initMouseEvent(type, true, true, window, 1,
      xPos, yPos,
      xPos, yPos,
      false, false, false, false,
      button, null);
    target.node().dispatchEvent(e);
  }

The members of the object ClientRect returned by getBoundingClientRect() are defined in the standard as float . But, if these values are passed to MouseEvent.InitMouseEvent they are converted to integers. This is where all these precision errors are originating.

One circumstance where the members of ClientRect may not be integers is when the zoom factor of the browser is < 1.
Try running the coverage.html in chrome, with zoom factor set to e.g. 75% and you may see some errors you don't otherwise see.

IE10 apparently returns fractional values like this:

can register callback using onClick() 
‣
AssertionError: was passed correct point: expected { x: 200.5, y: 200.5 } to deeply equal { x: 200, y: 200 }
   at Anonymous function (tests.js:14028:17)

by design: see here

Not sure why phantomjs is returning these non-integer values though, or why the test run under travis fails.

@softwords
Copy link
Author

Clean test now, but I have made the byIndex function the default key instead of noConstancy. noConstancy solves the issues around AnimateOnNextRender, but in other tests e.g. metaDataTests=>Dataset is passed in are expecting constancy by index to work... so there is a bit of contradiction.

Needs more discussion before pulling I think.

@bluong
Copy link
Contributor

bluong commented Oct 2, 2015

A huge step forward as the pull request is now a lot more reviewable 😃

I can review it on my end now. In the meantime, can you look to resolve the "plottable.d.ts" issues. Also, if you haven't already, can you submit a signed version of the CLA in the repo? Thanks!

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

bluong commented Oct 5, 2015

Hmm, if you merged in the other pull request into this one, it still seems that there is a huge diff in "plottable.d.ts"

@softwords
Copy link
Author

Yes, I know. But, when I look at the raw files of plottable.d.ts in this PR and in the develop branch, I can't see any difference at all to the naked eye, so it's very puzzling. No other diff tool (ie in Visual Studio or notepad++) sees them as different either. Could it be that, because the earlier commit 'Compiled Outputs', effectively threw the whole file away and wrote it again, that all diffs on subsequent commits accumulate that change? In other words, does git diff compare the 2 files absolutely ( regardless of their history) or does it somehow compute the diff by accumulating the changes in all the commits between them? I'll continue to research this.
Either way, we can't release this file into the develop branch.
Question: when you merge a PR - do you take the plottable.js and .d.ts included in the PR into develop, or do you recalculate them?

@softwords
Copy link
Author

fyi for anybody interested, I'm finding that the git support in VS2015 is too simplistic really for a project of this complexity, I'm running Atlassian sourceTree in parallel to VS, changes made in SourceTree do get immediately reflected in VS.

@bluong
Copy link
Contributor

bluong commented Oct 5, 2015

I believe that git diff only looks at the absolute changes without regarding the history

@bluong
Copy link
Contributor

bluong commented Oct 5, 2015

We do indeed take the .js / .d.ts file included in the PR since we don't currently have a way to compile these built files to the develop branch after-merge.

# Conflicts:
#	plottable.d.ts
@@ -4,9 +4,23 @@ module Plottable {

export type DatasetCallback = (dataset: Dataset) => void;

export class KeyFunctions {
protected static counter: number = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why protected?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, I'll change to private.

@bluong
Copy link
Contributor

bluong commented Oct 9, 2015

@softwords Yep, I think I see what you are saying. I am mainly suggesting that you make a JSFiddle so that I, other viewers, and our QE can have a proof-of-concept of this specific pull request change.

In interacting with your Scatter Plot example, it seems that this can work with some tweaking (Given the src attribute, it is hard to tell that the input "plottable.js" actually comes from pull request. We generally use rawgit.com for these situations.)

@bluong
Copy link
Contributor

bluong commented Oct 9, 2015

In terms of the overall pull request, most of my comments are essentially very small changes / cleaning up. My only concern revolves around the Scatter Plot change. Do we want to now make this change across the rest of the plots? Or do we just have it on Scatter Plot?

@softwords
Copy link
Author

Here's two variations on the previous fiddle:
http://jsfiddle.net/oaskqta0/6/
This one is built using the current PR. This uses the existing animateOnRender logic in BarPlot. There are no longer transitions on enter or exit. The main difference you'll see between constancy and noconstacy is that, with constancy, the bars remember their height. With noConstancy each bar grows from the origin in its new position every time. In both cases they do not transition to their new positions - they just jump - this is because of the extra drawstep inserted (with Null Animator) by the animateOnRender logic.
http://jsfiddle.net/oaskqta0/7/
this one is built from a new branch ( no PR) . It removes the animateOnRender logic from BarPlot. Now you can see the updated selection (ie those countries appearing in both datasets) transition to both their new height and new position. But, there is still no transition on the enter or exit selections, because the Animator can't get at these yet.

@softwords
Copy link
Author

http://jsfiddle.net/oaskqta0/8/
Same data, current PR, scatter chart. The enter() selection squirts in from top left. This is quite dramatic with noConstancy. The reason for this is that they are not initialised with any properties when they are created, so the transition to their ultimate values begins at 0,0. (This is a bit different to what happens now - the animateOnRender kills only the d attribute and retains the transform which fixes the x,y) . In the d3 enter/update/exit pattern, there is usually some special setup done on the entering elements to initialise various attributes even before they are transitioned to their final size and position. With a richer IAnimator interface, we can make a hook to do this.

# Conflicts:
#	plottable.d.ts
@bluong
Copy link
Contributor

bluong commented Oct 9, 2015

Do you plan on having the Plots.Bar scenario to be part of this pull request? Also, for that scenario, we usually have the first animation start from the baseline. Is that incorporated?

@softwords
Copy link
Author

Do you plan on having the Plots.Bar scenario to be part of this pull request?

I'll be guided by you as to how much / how little of this you want to introduce in this PR or delay to subsequent PR. This change is fundamental ( see the 3 steps I described earlier) , but of itself, doesn't add a lot of functionality. From your perspective, I can appreciate that you may want a better idea of where all this is heading before committing anything.
If you commit now, the thing people will see is a change in the animation behaviour of scatter plots. They will have some control over this, but the "squirting in from top-left" behaviour is what they'll see at first.
On the other hand, we could reduce this PR ( by taking out the change to ScatterPlot) so that it makes no impact on any existing code at all. But by doing that, setting KeyFunction will never have any effect.
Or, we can hold off while I work on the second step - which involves IAnimator changes - and roll it all in in one go. Then there will be more fine-grained control over animations, but it becomes a big PR.
My suggestion would be, let's put ScatterPlot back the way it was, QE won't see any difference, but we can then move forward on a clean page to build on this foundation.

@softwords
Copy link
Author

by the way, thanks for all your patience and assistance, it's much appreciated

@bluong
Copy link
Contributor

bluong commented Oct 9, 2015

In talking with QE, I think a solution that came up with was essentially to create a side branch that branches off of develop, and then you can make pull requests to that side branch. We'll then iterate on each of those pull requests and then once we feel that the side branch is ready to go, we hopefully simply merge that in. How does that sound to you?

In this regard, yes let's remove the Scatter Plot functionality and stick with only the KeyFunction idea. Place that functionality in a separate pull request only dealing with the Scatter Plot change.

To my understanding I don't think non-contributors can create branches on our end, I will create a branch at dataConstancy . If this makes sense, can you make a new pull request that targets that branch instead? If so, make have this pull request reference that pull request and vice versa.

@bluong
Copy link
Contributor

bluong commented Oct 9, 2015

I definitely want to shout my thanks back to you for sticking with our development cycle. Your contribution definitely looks really promising and I would like to find a way to include it into Plottable in the most effective manner.

@softwords
Copy link
Author

create a side branch that branches off of develop

that's sound like a good plan - can I suggest you call it objectConstancy, in keeping with Mike Bostock's terminology?

I'll wait to hear back on that.
Meantime, I'm hoping to see your changes to the compile sequence in the gruntfile soon, which (mysteriously) fixes my plottable.d.ts troubles.

@bluong
Copy link
Contributor

bluong commented Oct 9, 2015

I'll change the branch to objectConstancy.

In regards to the compile sequence, you can try testing your pull request by rebasing on my pull request, possibly. We are expecting that to merge by hopefully early next week.

@bluong
Copy link
Contributor

bluong commented Oct 10, 2015

Branch created, feel free to make a pull request to that branch

@bluong
Copy link
Contributor

bluong commented Oct 12, 2015

Just in case, Github does not allow the changing of the target branch on a pull request. Instead, you will need to create a new pull request with the same source branch and change the target branch to "objectConstancy".

@bluong
Copy link
Contributor

bluong commented Oct 12, 2015

Closing this pull request since it seems we have a reference to the new pull request.

@bluong bluong closed this Oct 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants