Issue 258910043: Adds the ability for the line chart to handle sparse data series. (Closed)
|
Created:
10 years, 10 months ago by charliea Modified:
10 years, 10 months ago CC:
tracing-review_chromium.org Base URL:
https://github.com/google/trace-viewer.git@master Visibility:
Public. |
DescriptionAdds the ability for the line chart to handle sparse data series.
This is necessary for the chart where we'll graph power consumption by
ms after vsync because not every frame has power measurements at the
same times after the vsync occurred.
BUG=#1108
R=nduca@google.com
Committed: https://chromium.googlesource.com/external/trace-viewer/+/8a22d0218a1831cba0642baab14753709c69416e
Patch Set 1 #Patch Set 2 : Fixes bug where series list is only calculated off of the first datum #
Total comments: 21
Patch Set 3 : Changes based on code review #Patch Set 4 : Made some test changes and used !== undefined instead of hasOwnProp #
Total comments: 1
MessagesTotal messages: 18
can you provide a screenshot what is graphed for your test case? its confusing to me what the visual display of sparsity is...
Sign in to reply to this message.
On 2015/07/21 17:06:04, nduca(google) wrote: > can you provide a screenshot what is graphed for your test case? its confusing > to me what the visual display of sparsity is... Sure thing - it looks like this: https://goo.gl/f4s2hU. I found another bug where the total list of data series was being calculated off of the first datum, which is a problem if the first datum doesn't have all of the series that might appear later on. I'm uploading a fix for this bug in the current patch set.
Sign in to reply to this message.
Also probably worth noting: I think that, in the long run, it makes sense to have the interface for the line chart look a lot more like this: http://c3js.org/samples/simple_xy_multiple.html. This would greatly simplify the logic in line_chart.html that accounts for multiple series. (And, actually, if we're registering votes, mine would be to just use a third-party charting library. I don't think that we have the time to ever match the quality of something like c3.js, and it's only 140kB to include anyhow.)
Sign in to reply to this message.
Agreed in the long haul about c3js. Its tough though: 140kb is not doable while we're inside chrome --- we need to make chrome://tracing an extension. That's something we wanna do, ned has thought a bit about it, but its kinda backburnered. If we got that sorted, then size constraints would be lessened.
Sign in to reply to this message.
On 2015/07/21 19:28:38, nduca(google) wrote: > Agreed in the long haul about c3js. Its tough though: 140kb is not doable while > we're inside chrome --- we need to make chrome://tracing an extension. That's > something we wanna do, ned has thought a bit about it, but its kinda > backburnered. If we got that sorted, then size constraints would be lessened. Ahhh, interesting: that makes sense. I forgot that any Javascript that we include has to ship with Chrome. Definitely agree that it would be nice to have this constraint lifted.
Sign in to reply to this message.
https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... File tracing/tracing/ui/base/line_chart.html (right): https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:193: * Returns a map of series key to the data (from 'this.data_') for I think you want to follow docstring conventions here... or use /* */ instead. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... File tracing/tracing/ui/base/line_chart_test.html (right): https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart_test.html:57: {x: 20, value2: 10}, woulda been clearer if you did value1: undefined but i notice you used hasOwnProperty. Any merits to just checking !== undefined in the real code, then you can pass undefined? That let you construct this out of map functions and not have to specially construct the dict. eg objToDatum => return {blah: x.foo, baz: y.baz} versus having to conditionally add baz. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart_test.html:66: seems like if we add support for this, we should clarify in the doc string [and verify] the cases where sparsity will fail. e.g. you can't have a lone point, and you can't have more than one "gap". Thats kinda complex to explain, so its probably important that we throw when we get that, right?
Sign in to reply to this message.
My question would be, is going to 0 the right thing to do with this sparse data? It's kinda misleading as, the battery usage didn't drop to zero suddenly. Should we instead leave a gap in the graph, or used a dash'd line to connect the known points together? Assume a linear draw connecting the known points? https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... File tracing/tracing/ui/base/line_chart.html (right): https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:121: var keys = d3.keys(keySet); is d3.keys the same as Object.keys? I think Object.keys would be clearer here as keySet isn't a d3 object. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:122: keys.splice(keys.indexOf('x'), 1); Why not just do if key === 'x' return in the forEach above? https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:138: this.seriesKeys_.forEach(function(k) { Can we rename k to key and d to data in here? https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:193: * Returns a map of series key to the data (from 'this.data_') for Drop the (from this.data_) as it's an implementation detail and isn't relevant where the keys come from. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:210: if (seriesKey == 'x') nit: === https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:214: singleSeriesDatum[seriesKey] = multiSeriesDatum[seriesKey]; dataBySeriesKey[seriesKey].push({ x: x, seriesKey: multiSeriesDatum[seriesKey] }); https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... File tracing/tracing/ui/base/line_chart_test.html (right): https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart_test.html:49: test('twoSparseSeries_firstValueSparse', function() { This looks like an instantiation test, can you add instantiate to the name? https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart_test.html:66: On 2015/07/21 23:34:45, nduca(google) wrote: > seems like if we add support for this, we should clarify in the doc string [and > verify] the cases where sparsity will fail. > > e.g. you can't have a lone point, and you can't have more than one "gap". Thats > kinda complex to explain, so its probably important that we throw when we get > that, right? Why can you only have one gap? I'd assume we can have multiple gaps in the data that we fill in.
Sign in to reply to this message.
https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... File tracing/tracing/ui/base/line_chart.html (right): https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:121: var keys = d3.keys(keySet); On 2015/07/22 00:16:49, dsinclair wrote: > is d3.keys the same as Object.keys? I think Object.keys would be clearer here as > keySet isn't a d3 object. Done. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:122: keys.splice(keys.indexOf('x'), 1); On 2015/07/22 00:16:49, dsinclair wrote: > Why not just do if key === 'x' return in the forEach above? I actually had this and changed it... but I like your way better. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:138: this.seriesKeys_.forEach(function(k) { On 2015/07/22 00:16:49, dsinclair wrote: > Can we rename k to key and d to data in here? Done. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:193: * Returns a map of series key to the data (from 'this.data_') for On 2015/07/21 23:34:45, nduca(google) wrote: > I think you want to follow docstring conventions here... or use /* */ instead. Done. Followed the docs at http://usejsdoc.org/tags-returns.html. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:193: * Returns a map of series key to the data (from 'this.data_') for On 2015/07/22 00:16:49, dsinclair wrote: > Drop the (from this.data_) as it's an implementation detail and isn't relevant > where the keys come from. Done. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:210: if (seriesKey == 'x') On 2015/07/22 00:16:49, dsinclair wrote: > nit: === Done. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:214: singleSeriesDatum[seriesKey] = multiSeriesDatum[seriesKey]; On 2015/07/22 00:16:49, dsinclair wrote: > dataBySeriesKey[seriesKey].push({ > x: x, > seriesKey: multiSeriesDatum[seriesKey] > }); This is what I originally tried, but it ends up thinking that the actual string "seriesKey" is the key if I do that :( https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... File tracing/tracing/ui/base/line_chart_test.html (right): https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart_test.html:49: test('twoSparseSeries_firstValueSparse', function() { On 2015/07/22 00:16:49, dsinclair wrote: > This looks like an instantiation test, can you add instantiate to the name? Added instantiation on all of them except for the brushing from indices test. https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart_test.html:57: {x: 20, value2: 10}, On 2015/07/21 23:34:45, nduca(google) wrote: > woulda been clearer if you did value1: undefined > > but i notice you used hasOwnProperty. Any merits to just checking !== undefined > in the real code, then you can pass undefined? That let you construct this out > of map functions and not have to specially construct the dict. eg objToDatum => > return {blah: x.foo, baz: y.baz} versus having to conditionally add baz. I'm not sure I understand where you're coming from. Could you understand why it'd be clearer if value1 was undefined? https://codereview.appspot.com/258910043/diff/20001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart_test.html:66: On 2015/07/22 00:16:49, dsinclair wrote: > On 2015/07/21 23:34:45, nduca(google) wrote: > > seems like if we add support for this, we should clarify in the doc string > [and > > verify] the cases where sparsity will fail. > > > > e.g. you can't have a lone point, and you can't have more than one "gap". > Thats > > kinda complex to explain, so its probably important that we throw when we get > > that, right? > > Why can you only have one gap? I'd assume we can have multiple gaps in the data > that we fill in. Yep, Dan's right - it's totally fine to have multiple "gaps" in the data. This is because all we're doing is drawing, for each series, lines between the data points that we *do* have for that series. In that sense, it's not really possible to have "gaps" in the data, because that gap will be filled by interpolating between the data points that we have. All we're changing here is that the value1 series doesn't have to have data points at all of the same x values that the value2 series does. This is in contrast with the old functionality, where the value1 series had to have a data point at x1 if the value2 series one there.
Sign in to reply to this message.
On 2015/07/22 00:16:49, dsinclair wrote: > My question would be, is going to 0 the right thing to do with this sparse data? > It's kinda misleading as, the battery usage didn't drop to zero suddenly. > > Should we instead leave a gap in the graph, or used a dash'd line to connect the > known points together? Assume a linear draw connecting the known points? > Didn't see an answer to the above in the reply.
Sign in to reply to this message.
> On 2015/07/21 23:34:45, nduca(google) wrote:
> > woulda been clearer if you did value1: undefined
> >
> > but i notice you used hasOwnProperty. Any merits to just checking !==
> undefined
> > in the real code, then you can pass undefined? That let you construct this
out
> > of map functions and not have to specially construct the dict. eg objToDatum
> =>
> > return {blah: x.foo, baz: y.baz} versus having to conditionally add baz.
>
> I'm not sure I understand where you're coming from. Could you understand why
> it'd be clearer if value1 was undefined?
(1) Because you have used hasOwnProperty, you cannot pass in items of the form
{x: 1, y: undefined}. That precludes using map to create the items. Instead, you
have to write your functions as
var item = {x: 1}
if (mySource.y !== undefined)
item.y = mySource.y
Thats less clear.
(2) Because you have used hasOwnProperty, your test case arrays read as follows:
{x: 10, value1: 100, value2: 50},
{x: 20, value2: 50}, // here you have to literally look in and see , "oh, its
value1 that is missing". It doesn't jump out in a visual scan.
That is particularly problematic because visually the only difference between
the one above and this one is which value is missing:
{x: 10, value1: 100, value2: 50},
{x: 20, value1: 50}, // here you have to literally look in and check the '1'
vs '2' to see which is missing.
This is less clear than the following:
{x: 10, value1: 100, value2: 50},
{x: 20, value1: undefined, value2: 50},
I argue this is clearer because every object has the same fields, and so it
jumps out at you that
As such, I really think you should be checking for non-undefinedness, instead of
using hasOwnProp. The rest of the codebase uses hasOwn througout to detect
presence, and we only use hasOwnProp in very very specific situations when it is
absolutely and certainly the right way to do things.
Sign in to reply to this message.
On 2015/07/22 13:52:38, dsinclair wrote: > On 2015/07/22 00:16:49, dsinclair wrote: > > My question would be, is going to 0 the right thing to do with this sparse > data? > > It's kinda misleading as, the battery usage didn't drop to zero suddenly. > > > > Should we instead leave a gap in the graph, or used a dash'd line to connect > the > > known points together? Assume a linear draw connecting the known points? > > > > Didn't see an answer to the above in the reply. We resolved this offline. To summarize, we are assuming a linear draw between the known points.
Sign in to reply to this message.
On 2015/07/22 16:46:15, nduca(google) wrote:
> (1) Because you have used hasOwnProperty, you cannot pass in items of the form
> {x: 1, y: undefined}. That precludes using map to create the items. Instead,
you
> have to write your functions as
>
> var item = {x: 1}
> if (mySource.y !== undefined)
> item.y = mySource.y
>
> Thats less clear.
Good point. I changed the code so that we're checking for undefined.
I don't have a strong opinion on whether one way to test it is clearer than
another, so I changed the tests to do it the way you suggested.
Sign in to reply to this message.
> > Good point. I changed the code so that we're checking for undefined. > > I don't have a strong opinion on whether one way to test it is clearer than > another, so I changed the tests to do it the way you suggested. Also worth noting: I changed the names of the actual series, as I thought that them having the same first 6 characters was hurting the test clarity.
Sign in to reply to this message.
/poke If you guys could take a look at this soon, I'd really appreciate it! This is blocking the work to get a line graph where each frame's power consumption is overlaid.
Sign in to reply to this message.
lg to me.
Sign in to reply to this message.
lgtm ... ive not given this a careful review but it looks solid https://codereview.appspot.com/258910043/diff/80001/tracing/tracing/ui/base/l... File tracing/tracing/ui/base/line_chart.html (right): https://codereview.appspot.com/258910043/diff/80001/tracing/tracing/ui/base/l... tracing/tracing/ui/base/line_chart.html:139: // Allow for sparse data // Comments are sentences that end in periods.
Sign in to reply to this message.
Message was sent while issue was closed.
Committed patchset #4 (id:80001) manually as 8a22d0218a1831cba0642baab14753709c69416e (presubmit successful).
Sign in to reply to this message.
|
