Skip to content

Non-fancy scattergl to work with dates #1021

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Oct 24, 2016
Merged

Non-fancy scattergl to work with dates #1021

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fac70b2
Admit date X axes for fast (non-fancy) scattergl
monfera Oct 7, 2016
a23756d
Admit date X axes for fast (non-fancy) scattergl hover tooltip too
monfera Oct 7, 2016
ab4ffbf
adding tests for fancy vs non-fancy scattergl
monfera Oct 10, 2016
f5d6113
separate the new date rendering tests from axes_test.js
monfera Oct 17, 2016
3c79068
Admit isDateTime === true values (e.g. date strings) in non-fancy sca…
monfera Oct 19, 2016
5b71b47
doing cheaper, probabilistic checks outside the scattergl conversion …
monfera Oct 20, 2016
46a6b6c
minor speedup - avoid some boolean checks in the scattergl conversion…
monfera Oct 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Admit date X axes for fast (non-fancy) scattergl
  • Loading branch information
@monfera
monfera committed Oct 7, 2016
commit fac70b2918fbe87c37c81ecaa1ced5be6c22a48c
5 changes: 3 additions & 2 deletions src/traces/scattergl/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ proto.handlePick = function(pickResult) {

// check if trace is fancy
proto.isFancy = function(options) {
if(this.scene.xaxis.type !== 'linear') return true;
if(this.scene.xaxis.type !== 'linear' && this.scene.xaxis.type !== 'date') return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

if(this.scene.yaxis.type !== 'linear') return true;

if(!options.x || !options.y) return true;
Expand Down Expand Up @@ -279,7 +279,8 @@ proto.updateFast = function(options) {
yy = y[i];

// check for isNaN is faster but doesn't skip over nulls
if(!isNumeric(xx) || !isNumeric(yy)) continue;
if(!isNumeric(yy)) continue;
if(!isNumeric(xx) && !(xx instanceof Date)) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Lib.isDateTime is what you need here.

I'm afraid it will be significantly slower than xx instanceof Date though.

Copy link
Contributor Author

@monfera monfera Oct 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@etpinard @alexcjohnson I'd just like to clarify something here. Testing with Lib.isDateTime is useful here as long as we actually do something with the broader set of options, in particular, string representations of dates. So there's got to be logic to check the type and dispatch to either straight-through processing (no conversion, i.e. Date objects that are handled now) or conversion from the string representations of dates to Date objects. Should I go ahead and do these things? As mentioned above, I'd keep the fast path fast by testing the first legitimate value only, and if it's suitable for straight-through processing (STP) then the entire array would be assumed to be so. (As @alexcjohnson mentioned I'd check for the first non-null value).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little surprised that epoch milliseconds are accepted as date data right now, I don't think our svg cartesian axes accept that, do they? But anyway I think your plan sounds good. Lib.dateTime2ms does short-circuit Date objects, but you could certainly shave off a little extra overhead by not going through it at all, and I think it's fine to assume (at least for these performance-oriented types) that all valid values in a given array have the same type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson sorry I conflated two things in the spur of the moment, acceptance of numeric values and the rendering of dates. If I pass on epoch milliseconds it'll render fine but it won't render the numbers as dates; it'll simply show the epoch milliseconds as numbers. I'll revise my comment.


idToIndex[pId++] = i;

Expand Down