Blue noise sampling #3531

rougier · 2018-11-02T21:35:55Z

Description

This PR implement blue noise sampling following proposal #2380. I'm not quite sure how to add the associated example in doc/examples.

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

Clean style in the spirit of PEP8
Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]
[If this is a new feature, it implements the following paper: ]

For reviewers

(Don't remove the checklist below.)

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.
Consider backporting the PR with @meeseeksdev backport to v0.14.x

pep8speaks · 2018-11-02T21:35:57Z

Hello @rougier! Thanks for updating the PR.

In the file doc/examples/utils/plot_noise.py, following are the PEP8 issues :

Line 17:1: E302 expected 2 blank lines, found 1
Line 25:1: E305 expected 2 blank lines after class or function definition, found 1
Line 28:34: E231 missing whitespace after ','
Line 29:14: E231 missing whitespace after ','
Line 29:22: E231 missing whitespace after ','
Line 34:10: E226 missing whitespace around arithmetic operator
Line 34:31: E226 missing whitespace around arithmetic operator
Line 34:36: E226 missing whitespace around arithmetic operator
Line 35:10: E226 missing whitespace around arithmetic operator
Line 35:31: E226 missing whitespace around arithmetic operator
Line 35:36: E226 missing whitespace around arithmetic operator
Line 37:1: W293 blank line contains whitespace
Line 40:14: E231 missing whitespace after ','
Line 40:22: E231 missing whitespace after ','
Line 41:1: W293 blank line contains whitespace

In the file skimage/util/noise.py, following are the PEP8 issues :

Line 8:1: E302 expected 2 blank lines, found 1
Line 16:48: W291 trailing whitespace
Line 44:25: E226 missing whitespace around arithmetic operator
Line 62:1: W293 blank line contains whitespace
Line 80:34: E231 missing whitespace after ','
Line 80:36: E226 missing whitespace around arithmetic operator
Line 83:80: E501 line too long (85 > 79 characters)
Line 90:1: W293 blank line contains whitespace

Comment last updated on November 04, 2018 at 17:41 Hours UTC

stefanv · 2018-11-03T00:36:06Z

Thanks, @rougier!

It looks like this should also be added to random_noise.

To add a gallery example, make the directory doc/examples/utils and add plot_noise.py in there. For an example of how those files are formatted, take a look at any of the other examples.

sciunto · 2018-11-03T08:00:11Z

skimage/util/noise.py

+        Two-dimensional domain (width x height) over which to sample noise
+    radius : float
+        Minimum distance between samples
+    k : int


Suggested change

k : int

k : int, optional

sciunto

Thanks for submitting! I made several comments, most of them on the style, and few suggestions on the code itself.

sciunto · 2018-11-03T08:00:26Z

skimage/util/noise.py

+        Minimum distance between samples
+    k : int
+        Limit of samples to choose before rejection (typically k = 30)
+    seed : int


Suggested change

seed : int

seed : int, optional

sciunto · 2018-11-03T08:04:59Z

skimage/util/noise.py

+
+    This function implements the method introduced in "Fast Poisson Disk
+    Sampling in Arbitrary Dimensions, Robert Bridson, Siggraph, 2007" for
+    generating (fast) blue noise.


When we want to add a reference, we add a specific section for that. Take a look here for instance: https://github.com/scikit-image/scikit-image/blob/master/skimage/filters/thresholding.py#L318 Note also how to put a DOI.

sciunto · 2018-11-03T08:05:43Z

skimage/util/noise.py

+
+    See also:
+    ---------
+    https://github.com/scikit-image/scikit-image/issues/2380


The See also section is generally not used for that. Here, I would not refer to the issue...

I removed it.

sciunto · 2018-11-03T08:06:44Z

skimage/util/noise.py


+def blue_noise(shape, radius=0.01, k=30, seed=None):
+    """
+    Function to add random noise of various types to a floating-point image.


Suggested change

Function to add random noise of various types to a floating-point image.

Add random noise of various types to a floating-point image.

Changed it (I wrongly copied it from the random_noise function)

sciunto · 2018-11-03T08:10:48Z

skimage/util/noise.py

+
+    """
+
+    def squared_distance(p0, p1):


Can't we use https://docs.scipy.org/doc/scipy-0.14.0/reference/generated/scipy.spatial.distance.euclidean.html instead?

That would be a bit overkill, no?

sciunto · 2018-11-03T08:17:12Z

skimage/util/noise.py

+    def in_limits(p):
+        return 0 <= p[0] < width and 0 <= p[1] < height
+
+    def neighborhood(shape, index, n=2):


Can you put comment to explain what the function is doing please?

I've added docstring for all of them.

sciunto · 2018-11-03T08:19:06Z

skimage/util/noise.py

+        I.remove([row, col])
+        return I
+
+    def in_neighborhood(p):


Suggested change

def in_neighborhood(p):

def in_neighborhood(p, squared_radius):

You're right. I've changed it but in the process some other functions are using implicit global variables. Would you prefer I tried to remove every inside function? I might be able to remove most of them (but not all).

I think some of them are not necessary, especially the short ones, So, +1!

sciunto · 2018-11-03T08:19:17Z

skimage/util/noise.py

+        del points[i]
+        Q = random_point_around(p, k)
+        for q in Q:
+            if in_limits(q) and not in_neighborhood(q):


Suggested change

if in_limits(q) and not in_neighborhood(q):

if in_limits(q) and not in_neighborhood(q, squared_radius):

sciunto · 2018-11-03T08:19:49Z

skimage/util/noise.py

+        i = np.random.randint(len(points))
+        p = points[i]
+        del points[i]
+        Q = random_point_around(p, k)


Suggested change

Q = random_point_around(p, k)

Q = random_point_around(p, radius, k)

sciunto · 2018-11-03T08:20:44Z

skimage/util/noise.py

+        If provided, this will set the random seed before generating noise,
+        for valid pseudo-random comparisons.
+
+    Notes:


Dont use : signs in headers please :)

Not sure I get this one...

Sorry, I wanted to say title.

All titles must look like

title -----

not

title: ------

:)

rougier · 2018-11-03T13:29:32Z

I've started to modify the source according to review but I think my code is too complicated and could probably be simplified. I'll try to look into that.

sciunto · 2018-11-03T17:29:34Z

Sounds great! Thanks @rougier

sciunto · 2018-11-03T17:52:13Z

skimage/util/noise.py

+    ----------
+
+    .. [1] Fast Poisson Disk Sampling in Arbitrary Dimensions, Robert Bridson,
+           Siggraph, 2007. :DOI: 10.1145/1278780.1278807


Suggested change

Siggraph, 2007. :DOI: 10.1145/1278780.1278807

Siggraph, 2007. :DOI:`10.1145/1278780.1278807`

…mulbreh/bridson

rougier · 2018-11-03T18:09:47Z

I've found the implementation by @emulbreh (see https://github.com/emulbreh/bridson, MIT License) to be more clear and probably faster. I've slightly adapted it.

sciunto · 2018-11-04T15:39:02Z

skimage/util/noise.py

+        queue.pop()
+        for _ in range(k):
+
+            # WARNING: p is not uniform sampled but we can live with it here


There is still this line I don't understand, and you didn't reply to me about my suggestion

Sorry, I did not see it. I need to pick k points around p in a torus (r,2r) and I used uniform polar coordinates introducing a bias when converted into Cartesian coordinates. I'll fix it and remove the comment.

And the implementation by @emulbreh is right actually.

I'm not sure about the suggestion you're referring to

sciunto · 2018-11-05T06:49:02Z

The main task that remains is to right few unittests. This is necessary to include a new feature in scikit-image. Are you familiar with this?

rougier · 2018-11-05T14:37:28Z

Yes. I will write some tests but I need to find relevant ones. Do I need to check both for potentially wrong arguments (this would require additional code for checking correctness of arguments such as for example negative sizes) and also expected statistical properties (approximate number of samples, mean distance between samples, etc.)?

sciunto · 2018-11-08T06:28:26Z

We usually do not perform extensive checks of parameters (you can have a look at other functions). IMO, it is not necessary to add more checks. I suggest to start by covering the existing code. Eventually, if other reviewers ask, we can add more.

emmanuelle · 2018-11-10T14:26:46Z

Hi sorry to jump in late. This code is very nice and I love the stippling application, but does it fit in the scope of scikit-image? I can't think of an application in biology, physics, astronomy etc. where one could use blue noise. If I'm wrong (please prove me wrong :-), the gallery example should try to show such a use case. Something like determining markers for a segmentation?

soupault · 2018-11-10T15:28:52Z

A followup question: may it be of interest for SciPy integration?

rougier · 2018-11-10T20:36:24Z

@emmanuelle No opinion on that. I'm fine if you prefer not to merge the PR.

emmanuelle · 2018-11-10T20:50:32Z

@rougier let's wait for other opinions; in #2380 both @jni and @JDWarner said that they saw places in the package where this could be used.

But perhaps we need to be more explicit about the scope of scikit-image, for example in the contributers documentation (and I'm sure different devs have a slightly different view of the scope of the package: is it mostly for scientific applications? Also for some computer vision applications? Graphics applications? This makes it all the more important to talk and agree about the scope of scikit-image).

emmanuelle · 2018-11-24T15:57:54Z

@jni your thoughts here would be welcome

Added blue noise sampling

0662db8

rougier added 2 commits November 3, 2018 08:20

Added usage example

140637b

More specific docstring

7b3387e

sciunto reviewed Nov 3, 2018

View reviewed changes

sciunto added type: new feature 🧐 Needs review labels Nov 3, 2018

rougier added 2 commits November 3, 2018 13:45

Added comments

6ce63ef

Bug fix

409c310

sciunto reviewed Nov 3, 2018

View reviewed changes

Complete overhaul adapting the implementation by https://github.com/e…

e08a0e3

…mulbreh/bridson

sciunto reviewed Nov 4, 2018

View reviewed changes

Fixed non-uniform sampling around p

8299c96

sciunto added ⚖️ Needs decision and removed 🧐 Needs review labels Dec 12, 2018

soupault requested a review from jni August 4, 2019 10:57

Base automatically changed from master to main February 18, 2021 18:23

mkcor mentioned this pull request Feb 22, 2021

2021's calendar of community management #5169

Closed

tupui mentioned this pull request Jul 29, 2021

ENH: Poisson Disk sampling for QMC scipy/scipy#13918

Merged

grlee77 added 🙏 Feature request and removed type: new feature labels Feb 22, 2022

mkcor mentioned this pull request Feb 28, 2022

2022's calendar of community management #6165

Closed

	Function to add random noise of various types to a floating-point image.
	Add random noise of various types to a floating-point image.

	def in_neighborhood(p):
	def in_neighborhood(p, squared_radius):

	if in_limits(q) and not in_neighborhood(q):
	if in_limits(q) and not in_neighborhood(q, squared_radius):

	Q = random_point_around(p, k)
	Q = random_point_around(p, radius, k)

	Siggraph, 2007. :DOI: 10.1145/1278780.1278807
	Siggraph, 2007. :DOI:`10.1145/1278780.1278807`

Blue noise sampling #3531

Are you sure you want to change the base?

Blue noise sampling #3531

Conversation

rougier commented Nov 2, 2018 • edited by sciunto Loading

Description

Checklist

References

For reviewers

pep8speaks commented Nov 2, 2018 • edited Loading

Comment last updated on November 04, 2018 at 17:41 Hours UTC

stefanv commented Nov 3, 2018

Choose a reason for hiding this comment

sciunto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sciunto Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

rougier commented Nov 3, 2018

sciunto commented Nov 3, 2018

Choose a reason for hiding this comment

rougier commented Nov 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sciunto commented Nov 5, 2018

rougier commented Nov 5, 2018

sciunto commented Nov 8, 2018

emmanuelle commented Nov 10, 2018

soupault commented Nov 10, 2018

rougier commented Nov 10, 2018

emmanuelle commented Nov 10, 2018

emmanuelle commented Nov 24, 2018 • edited Loading

rougier commented Nov 2, 2018 •

edited by sciunto

Loading

pep8speaks commented Nov 2, 2018 •

edited

Loading

sciunto Nov 3, 2018 •

edited

Loading

emmanuelle commented Nov 24, 2018 •

edited

Loading