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

Add functions resample() and smooth() #92

Closed

Conversation

danielbeardsley
Copy link
Contributor

Functions: add resample() for performance and sanity

  • Drastically speeds up further calcuations on the returned series
  • Makes it much easier to have a consistent datapoints / pixels ratio
    for movingAverage() and friends.

Functions: add smooth() as a movingAverage over pixels

  • Internally does a movingAverage(resample()).
  • Provides consistent smoothing over a given number of graph pixels,
    instead of a number of datapoints of arbitrary time width.
  • Still provides consistent smoothing when there are fewer datapoints
    than pixels.

* Drastically speeds up further calcuations on the returned series
* Makes it much easier to have a consistent datapoints / pixels ratio
  for movingAverage() and friends.
* Internally does a movingAverage(resample()).
* Provides consistent smoothing over a given number of graph pixels,
  instead of a number of datapoints of arbitrary time width.
* Still provides consistent smoothing when there are fewer datapoints
  than pixels.
@danielbeardsley
Copy link
Contributor Author

We've (iFixit) been using this in production for a while and it works great.
using smooth(some.stat.rate) makes way more sense (and is way faster) than movingAverage(some.stat.rate, 40) and then having to constantly adjust the 40 number as you view different time-scales.

@Dieterbe
Copy link
Contributor

looks interesting, but I wonder if there's any overlap with consolidateBy()

@client9
Copy link

client9 commented Nov 18, 2013

FYI --
@Dieterbe Right now changing the width of a graph (&width) has no effect on JSON output. Therefore consolidateBy will also have no effect, as the resampling for images is done in another path, only for images.

With a large number of points, I'm able to crash a few browsers using client-side rendering, so a re-sampling would be great.

If width and current resampling code could be moved out and re-used in the json output (i.e. no width = current system, if width, then resample to that many number of points.). Then consolidate by could be re-used, and no new functions are needed.

thoughts? I might be able to find some time to do this.

nickg

@client9
Copy link

client9 commented Nov 18, 2013

See also #153

@Dieterbe
Copy link
Contributor

I was having the same problem, I'm using https://github.com/vimeo/timeserieswidget/ to do client side rendering, and over a long period of time there's indeed too many datapoints and performance degrades and can indeed lead to browser crashes.

I think it's totally reasonable to have a width argument for json output (and other data outputs), as it gives a hint as to how many pixels/datapoints the client side renderer wants to draw.

I think all current consolidation should also work for raw outputs (see #153). Your suggestion of sampling instead of consolidating seems reasonable (for both png and raw) as that will allow even better (backend) performance, at the expense of some accuracy.

@drawks
Copy link
Member

drawks commented Jul 2, 2014

The consolidation portion of this for raw/json outputs appears to be addressed in 7adc7f4 I think that offering a sample mode vs avg could be useful though. I'll tag this for the 0.10.0 milestone. I'd like to see some tests for this before merging though.

@drawks drawks added this to the 0.10.0 milestone Jul 2, 2014
@danielbeardsley
Copy link
Contributor Author

We've been using these functions in production for a long time now and they've been great and address a real issue: speeding up slow calculations with large numbers of points, and time-scale independent smoothing of a series over a given number of pixels.

Three things I would like to find the time to do:

  • Properly rewrite the series names so they come out as smooth(blah) instead of movingAverage(resample(blah))
  • Add some tests
  • Have the smooth() function respect a query param like smoothPixels=5 so smoothing can be adjusted on the whole graph at once instead of editing the argument in each data series.

@obfuscurity
Copy link
Member

FYI I'm going to attempt to port this over to the master branch but this will not go into the 0.9.x branch.

obfuscurity added a commit to obfuscurity/graphite-web that referenced this pull request Aug 22, 2016
@obfuscurity
Copy link
Member

Closing this one out, let's move any discussion over to #1661.

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

5 participants