Skip to content

Allow pre-parsed data (to scale id's)#6768

Merged
etimberg merged 7 commits intochartjs:masterfrom
kurkle:pre-parsed
Dec 7, 2019
Merged

Allow pre-parsed data (to scale id's)#6768
etimberg merged 7 commits intochartjs:masterfrom
kurkle:pre-parsed

Conversation

@kurkle
Copy link
Member

@kurkle kurkle commented Nov 19, 2019

The

set _parsed in the Element directly to reference that original user provided object

From #6766 (comment)

Could be this simple.

Data needs to be array of {x-scale-id: value, y-scale-id: value}

Todo:

  • docs
  • partial updates (account for start / count)

@benmccann
Copy link
Contributor

We'll probably want to update the default scale IDs

I also wonder if this would allow us to remove Scale._parse or at least TimeScale._parse. We could simply have the user change the scale ID from x to t instead of parsing both attributes

@kurkle
Copy link
Member Author

kurkle commented Nov 19, 2019

I'd actually remove support for t. and Scale._parseObject.
Scale._parse is still needed for un-parsed data.

Could also have the input option, input: {x: 't'}// scale-id: input_property

@kurkle kurkle marked this pull request as ready for review November 19, 2019 18:50
etimberg
etimberg previously approved these changes Nov 20, 2019
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

cool! I'm eager to see how this helps the uPlot benchmark :-)

@kurkle
Copy link
Member Author

kurkle commented Nov 21, 2019

Master: (averages around 270ms)

image

master + this (and parsed: true): (averages around 240ms)

image

So the difference is around 30 ms, which is almost 10% now!
I like the fact that prepData, that prepares the benchmark is getting to the top .

Edit:
For reference, on this same machine:
2.9.3 ~650ms.
2.8.0 ~1200ms

etimberg
etimberg previously approved these changes Nov 21, 2019
@benmccann
Copy link
Contributor

I think float bar probably won't work because the _custom field won't get populated

@kurkle
Copy link
Member Author

kurkle commented Nov 22, 2019

I think float bar probably won't work because the _custom field won't get populated

If the user provides the _custom as expected for float bars, then it would work.
Maybe there should be a bit more info. Like "Any custom parsing done by the controller needs to be replicated in this mode?"

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

I think we probably need some docs about scale parsing. E.g. the time scale will require the user to pass milliseconds since the epoch if this option is enabled

We can ignore _custom in this PR and address it later. I think making the user specify _custom is a bit of an ugly API and we can probably come up with something nicer from the user perspective

benmccann
benmccann previously approved these changes Dec 6, 2019
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

just the two minor docs comments, but looks good to me

@etimberg etimberg merged commit 53c6c61 into chartjs:master Dec 7, 2019
@kurkle kurkle deleted the pre-parsed branch December 7, 2019 05:35
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