Skip to content

Internal data by axis instead of scale id#6912

Merged
etimberg merged 4 commits intochartjs:masterfrom
kurkle:axis-instead-of-id
Jan 6, 2020
Merged

Internal data by axis instead of scale id#6912
etimberg merged 4 commits intochartjs:masterfrom
kurkle:axis-instead-of-id

Conversation

@kurkle
Copy link
Copy Markdown
Member

@kurkle kurkle commented Jan 5, 2020

Relevant part of parseObjectData after babel:
pr

    for (i = 0, ilen = count; i < ilen; ++i) {
      index = i + start;
      item = data[index];
      parsed[i] = {
        x: xScale._parseObject(item, 'x', index),
        y: yScale._parseObject(item, 'y', index)
      };
    }

Locally run tests:

100 runs done in 15430ms. Average: 154ms, min: 92ms, max: 516ms, variation: 424ms
100 runs done in 16681ms. Average: 166ms, min: 87ms, max: 867ms, variation: 780ms

In plunker:

50 runs done in 6431ms. Average: 128ms, min: 87ms, max: 441ms, variation: 354ms

master

    for (i = 0, ilen = count; i < ilen; ++i) {
      var _parsed$i3;

      index = i + start;
      item = data[index];
      parsed[i] = (_parsed$i3 = {}, _defineProperty(_parsed$i3, xId, xScale._parseObject(item, 'x', index)), _defineProperty(_parsed$i3, yId, yScale._parseObject(item, 'y', index)), _parsed$i3);
    }

Locally run tests:

100 runs done in 19970ms. Average: 199ms, min: 118ms, max: 539ms, variation: 421ms
100 runs done in 22556ms. Average: 225ms, min: 109ms, max: 863ms, variation: 754ms

In plunker:

50 runs done in 8775ms. Average: 175ms, min: 118ms, max: 461ms, variation: 343ms

plunker

(all tests run on chrome 79)

etimberg
etimberg previously approved these changes Jan 5, 2020
Copy link
Copy Markdown
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I didn’t see anything wrong. Just to be safe though, can we test with a chart that has extra x and y axes? Since the default axis IDs are now x and y I want to be sure we got all the spots

@benmccann
Copy link
Copy Markdown
Contributor

If you have two y scales, y1 and y2, won't they both get parsed to y and the second one will override the data from the first?

@kurkle
Copy link
Copy Markdown
Member Author

kurkle commented Jan 5, 2020

The parsed is per dataset (unless parsing is disabled and supplying same data to multiple datasets)
(and a dataset is obviously bound to single y-scale)
The parsing: false case suffers from this, you can't have data like {time: 123414, line: 10, bar: 15} and just give the scales id:s 'time', 'line' and 'bar'

benmccann
benmccann previously approved these changes Jan 5, 2020
@kurkle
Copy link
Copy Markdown
Member Author

kurkle commented Jan 5, 2020

I'll check re-check docs etc on this, lets not merge yet.

@kurkle kurkle dismissed stale reviews from benmccann and etimberg via f545c06 January 5, 2020 19:26
@kurkle
Copy link
Copy Markdown
Member Author

kurkle commented Jan 5, 2020

Ok, ready for another review

@etimberg etimberg merged commit b5d5ed9 into chartjs:master Jan 6, 2020
@kurkle kurkle deleted the axis-instead-of-id branch February 19, 2020 18:26
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