-
Notifications
You must be signed in to change notification settings - Fork 187
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
d3.blur #151
Conversation
Amazing! |
I'm pushing a new version which reduces the problem of the extremities being "dampened" by duplicating (radius times) the value at each extremity. Bonus, this approach makes it even faster (we're now under 50% of the original time). On the random image and a large radius, it creates a visible artifact: the values on the edges tends to impose themselves. Not strictly better or worse than the original, only different. (The solution when this is a problem is to compose two blurs, the first with a small blur radius, then a larger one.) |
Fascinating. Yeah I thought there was something going on with the extremities being "dampened", with the Gaussian blur approach in general. It's great that your implementation accounts for this! Some interesting related reading that might touch upon this issue: |
Another use case for this type of blurring: "hue" blurring on an image. We first read each pixel's value in the Lab model (or HCL, HSL…), then blur a and b, and reassemble the image: https://observablehq.com/@fil/hue-blur |
Finally I don't think the rgba thing is in the scope of this pull-request. It creates a bit of complications (stride, channel, dtype) "just" for specific optimizations, and it's possible to do without. |
I've added a notion that the shape created around a dot (or single pixel) should be circular(ish), so that d3-contours are more "round" than "square". Unfortunately it seems that the stackblur's distribution profile results in shapes that are a bit squarish: So I think I have to either go back on that part, offer the two possibilities, or implement it separately in d3-contours. |
I went back to the original algorithm (to have "circular" shapes for d3-contour), and introduced non-integer radius, which help blur "just a little", for example with blur().radius(0.1). I feel this PR is now complete. |
Wow, this is awesome! Thank you @Fil for your work on this. |
Another demo with the "cloud contours" notebook: https://observablehq.com/d/c9d89106eafefb4f |
Wow that is so cool! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a fairly thorough re-review. Found a few small nits.
Overall this is looking great! Really looking forward to see this PR land.
README.md
Outdated
@@ -536,6 +536,39 @@ Returns an array of arrays, where the *i*th array contains the *i*th element fro | |||
d3.zip([1, 2], [3, 4]); // returns [[1, 3], [2, 4]] | |||
``` | |||
|
|||
### Blur | |||
|
|||
<a name="blur" href="#blur">#</a> d3.<b>blur</b>() · [Source](https://github.com/d3/d3-array/blob/master/src/blur.js), [Examples](https://observablehq.com/@d3/d3-blur) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice the examples link does not resolve https://observablehq.com/@d3/d3-blur
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll publish some examples if the PR lands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the review curran!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My pleasure! Thanks for your great work on this feature.
Nice. This PR is super cool. Can't wait for it to be merged! Is there any more work needed here to complete the PR? Looks ready to me. |
If this could improve |
This is a somewhat superficial reaction, but I wonder what this would look like without the fluent-style API. So, instead of: const blurred = d3.blur()
.radius(radius)
.width(detail.width)
.value(d => d.b)
(pixels); You might say: const blurred = d3.blur(pixels, radius, detail.width, d => d.b); The other thing that feels weird is that d3-array methods are, generally, 1-dimensional methods. Here d3.blur is two-dimensional, expressed in terms of x and y (or horizontal and vertical). This makes me think that this method should perhaps live in a different module that’s more related to 2D images. Alternatively, would it be possible to have a slightly lower-level API with an optional stride argument instead (where stride = 1 for “horizontal” and stride = width? for vertical), similar to ndarray? |
Another consideration is whether d3.blur should happen in-place, and eliminate the accessor. Would that be possible or does it require a copy? const blurred = Float64Array.from(pixels, d => d.b);
d3.blur(blurred, radius, detail.width); |
It's also 1-d, with a no-op vertical blur in the default case. Internally there are two copies, that we swap at each iteration, so I'm not sure it can be done "in place", though maybe by keeping track of the parity of swaps it might be possible to save 1 copy allocation. In practice though I think I prefer this "immutable" version. (As I needed it on several experiments I was just at this instant publishing it on github and npm (as array-blur) — but happy to revert that if it can be integrated into D3 in some way.) |
Thrilled to see that this was published in https://github.com/Fil/array-blur ! |
traversing thru the observablehq forums lead the thru the d3 docs and back to here. was looking for EMWA or movingAverage for 1d data, and found array-blur instead (which is actually more appropriate for my use case anyway :)). great work fil! are the outstanding blockers around whether or not such functionality is relevant/welcome here, or rather outstanding qualms with the interface? just curious :) |
Thank you, Christopher! While it may never be included in d3-array, you can use this function by calling the array-blur module. I've packaged it just like the d3 modules, which means you can even attach it to the d3 symbol if you like, with: const d3 = require("d3@7", "array-blur@1");
const blur = d3.blur().radius(1.3).width(w);
blur(pixels); // blurred image Another possibility is that it would exist as a stand-alone module d3-blur (i.e. a renaming of array-blur); this would keep this 2D notion separate from d3-array, and allow d3-contour to use this faster and better blurring method. I've made this notebook to summarize various use cases, in particular how to use it with Observable Plot (by passing blur().radius(r) to Plot.mapY): |
* blur[12] * better fractional blur radius * fix blur2 * lift up radius checks * tiny optimization * blur2 takes {data, width, height} * skip blurh for blur1 * blurImage * remove unnecessary max * simpler inclusive stop * DRY * make height redundant * rename blur1 to blur Co-authored-by: Philippe Rivière <[email protected]>
We'll need two documentation notebooks:
|
fixes #56
see https://observablehq.com/@fil/moving-average-blur
stackblur algo (a bit faster, but more difficult to understand)