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

Adds ability to smooth lines for line and area charts #3273

Merged
merged 15 commits into from
Apr 7, 2015

Conversation

stormpython
Copy link
Contributor

Closes #825.

Adds ability to smooth lines in line chart.

@rashidkpc
Copy link
Contributor

Can you also add this to the area chart since they're so similar? There's no interpolate() call in there at the moment, but I gave it a shot and it seemed to work swimmingly.

@stormpython stormpython changed the title Adds ability to smooth lines for line chart Adds ability to smooth lines for line and area charts Mar 6, 2015
@stormpython
Copy link
Contributor Author

@rashidkpc done.

@w33ble
Copy link
Contributor

w33ble commented Mar 6, 2015

Seems like the bound padding might need to be increased when using smooth interpolation, as the lines can extend outside the bounds of the view.

screenshot 2015-03-06 11 17 13

screenshot 2015-03-06 11 19 02

@w33ble
Copy link
Contributor

w33ble commented Mar 6, 2015

Can you fix the UI a little while you're in here too? Specifically, just add the form-control class to the mode select, so they match...

screenshot 2015-03-06 11 47 19

@w33ble w33ble assigned stormpython and unassigned w33ble Mar 6, 2015
@w33ble w33ble removed the review label Mar 6, 2015
…going outside of the chart bounds, added form-control class to mode selector in area chart
@stormpython stormpython assigned w33ble and unassigned stormpython Mar 6, 2015
@stormpython
Copy link
Contributor Author

@w33ble Increased the line tension and added the form-control class.

@@ -0,0 +1,7 @@
<div>
<label>
Line Interpolation
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we s/Interpolation/Smoothing this? I don't think most people know what interpolation is. Also, if there's only on or off (linear or smooth) and we don't anticipate adding more options than that why not just make it a checkbox?

@rashidkpc
Copy link
Contributor

A bit wacky on percentage charts. Note the top of the chart

image

Conflicts:
	src/kibana/plugins/vis_types/vislib/editors/line.html
	src/kibana/plugins/vis_types/vislib/line.js
@stormpython
Copy link
Contributor Author

Renamed Line Interpolation to Line Smoothing. Removed the upper buffer from the svg clipPath on area charts since it was not needed. This solves the issue of lines in area charts going off the y axis.

@w33ble w33ble assigned stormpython and unassigned w33ble Mar 19, 2015
@stormpython
Copy link
Contributor Author

@w33ble An algorithm calculates the curvature of the line. If we wanted to know how curvy the lines would be, we'd need to do some measurement at every possible point that is used to calculate the position of each point on a line. Can you show me an example where increasing the y max value by 1 still shows lines being cut-off? I will decrease the yMin values when using the data extents by 1 to make sure this doesn't happen on the lower end.

If you can find or think of a better solution, let me know. As of now, this seems as good a fix as any.

@stormpython
Copy link
Contributor Author

Ok, it would appear that the solution to the lines going off the chart is not to use the cardinal option for smoothing, but use the monotone option which does all the smoothing but without the added fact of lines overshooting the axis.

@rashidkpc
Copy link
Contributor

@stormpython Can you merge master on this one?

@w33ble
Copy link
Contributor

w33ble commented Apr 2, 2015

The new smoothing tends to leave the border of the vis less, but it still happens (not the bottom of the chart).

image

The smoothing is less "smooth" now as a result though (note the very pointed peaks here). Perhaps this could perhaps be solved by offering users the ability to pick their smoothing method?

@rashidkpc how much do we care that the lines leave the border of the svg? It seems to happen regardless, and seems like a tricky problem to solve - it's the number one reason this PR keeps getting sent back.

Conflicts:
	src/kibana/plugins/vis_types/vislib/line.js
@stormpython
Copy link
Contributor Author

@w33ble the lines aren't actually leaving the bottom of the chart there. It only appears to since there are points there with zero values. It is simply going through the point of the circle. The reason it appears cut off is because part of the circle is cut-off, and that is due to the clip path that is set. Half the circle is above the 0 line and half is below. I don't really consider this to be a bad thing.

As far as the smoothness goes, I can reduce the tension on the line to make it more curvy. The very pointed peak is so the line goes through the point and does not overshoot it. This will happen no matter what as the algorithm is meant to intersect the points.

@rashidkpc
Copy link
Contributor

I personally don't much care that they leave the plot. If you choose to have smooth lines you choose to a lie :-) Given that line smoothing is very much for presentation I'd vote to prefer presentation (smoother lines) over accuracy (not going off the chart)

@stormpython
Copy link
Contributor Author

So, reducing the tension doesn't actually change the slope of the line between points by much. So I could either leave monotone the way it is or use cardinal (and have times where the lines go off the scale).

@w33ble
Copy link
Contributor

w33ble commented Apr 6, 2015

I liked how cardinal looked a lot better. I think most datasets probably look bad with monotone, but I'm open to letting the user choose (but defaulting to cardinal).

@w33ble w33ble assigned stormpython and unassigned w33ble Apr 6, 2015
@w33ble w33ble removed the review label Apr 6, 2015
…moothlines to cardinal, removing the tickScale function from the .nice calculation in the y axis
@stormpython
Copy link
Contributor Author

@w33ble after talking with @rashidkpc, he agreed with you that cardinal is a much better line interpolation algorithm than monotone. I've changed the drop down option to a checkbox option and sticking with cardinal. Since this pull should be getting in with #3464, users will have the ability to change the min and max y axis values if parts of lines get clipped.

@stormpython stormpython assigned w33ble and unassigned stormpython Apr 7, 2015
w33ble added a commit that referenced this pull request Apr 7, 2015
Adds ability to smooth lines for line and area charts
@w33ble w33ble merged commit 5b1c786 into elastic:master Apr 7, 2015
@stormpython stormpython deleted the line-smoothing branch June 19, 2015 20:01
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.

Line smoothing
3 participants