Added lower bound option to UniformHypersphere dist #799

xchoo · 2015-07-26T15:51:48Z

Added low and high options to UniformHypersphere distribution.

arvoelke · 2015-07-26T17:16:14Z

nengo/dists.py

@@ -156,8 +156,10 @@ class UniformHypersphere(Distribution):

    """

-    def __init__(self, surface=False):
+    def __init__(self, low=0, high=1, surface=False):


Docstring needs to be updated with these parameters, along with an explanation of how they affect the distribution.

arvoelke · 2015-07-26T17:17:28Z

Missing unit test.

xchoo · 2015-07-26T17:18:20Z

Haha. Yeah. I realized after making the pull request that I didn't do the tests for them. 😆

jgosmann · 2015-07-26T19:20:01Z

Doesn't this make the surface parameter redundant because you could set low = high = 1.?

arvoelke · 2015-07-26T19:46:33Z

Edit: Ignore my previous comment. I think you are right. 😄

For backwards-compatibility I wouldn't want to remove the surface parameter, though.

xchoo · 2015-07-26T20:35:02Z

Yeah. There's some interaction between the low, high and surface parameters that I didn't consider. I will fix that too.

tcstewar · 2015-07-26T21:25:56Z

Hmm, if I'm interpretting this right, setting low=0.5 and high=0.75 would lead to sample points that have a radius between 0.5^(1/D) and 0.75^(1/D) (but still uniformly distributed). Is that right? If so, it might be clearer to actually specify the radius, so that the parameters have a clearer meaning. Something like:

samples *= rng.uniform(low=self.low ** d, high=self.high ** d, size=(n, 1)) ** (1.0 / d)

That said, I'm not sure what the situation is you want these parameters for, so maybe they're more useful the way you have them -- I just thought I'd point out the possibility. :)

xchoo · 2015-07-26T21:44:38Z

Oh. I think you are right. haha

xchoo · 2015-07-26T22:12:19Z

Alrighty. I think I've addressed the issues listed here.

tcstewar · 2015-07-26T22:33:44Z

nengo/dists.py

+        self.high = max(high, self.low)
+
+        if surface:
+            self.low = self.high


This logic (of the interaction between high, low, and surface) should be documented in the parameters

It is? (well, maybe not the surface one, but the rest are?)

jgosmann · 2015-07-27T00:20:40Z

I don't like, the interaction of the arguments and the possible to pass nonsensical combinations. With surface=False the vector length is < high, but with surface=True the vector length is <= high. Also, with surface=True the value of low has no effect.

Not sure if there is a good way to work around this. Maybe deprecate surface to remove it in a future version? Or remove high and allow to pass in the value to surface?

tcstewar · 2015-07-27T01:30:20Z

I don't like, the interaction of the arguments and the possible to pass nonsensical combinations. With surface=False the vector length is < high, but with surface=True the vector length is <= high. Also, with surface=True the value of low has no effect.

That's my concern as well. And right now, the only documentation about this interaction is that "Values provided out of this range will be corrected", which i don't think is clear enough about what this interaction will do.

I'm still not sure what the use case is for this change, but one possibility would be to get rid of high as a parameter. In most of the use cases I can think of, high is redundant with radius, and is just a multiply anyway. So that might be sufficient.

@xchoo , what is the use case you're thinking of for this change? Would it work without high?

xchoo · 2015-07-27T02:29:45Z

Yup! It would work without high. Although, if that's the case, I would move to rename UniformHypersphere to UniformUnitHypersphere.

jgosmann · 2015-07-27T02:32:24Z

But it's only unit if the radius is 1 ...

xchoo · 2015-07-27T02:33:57Z

If you leave out high, the max magnitude of the distribution will be 1. In that case, if you set surface=True, the distribution will ignore the low value and put all the points on the surface of the hypersphere.

xchoo · 2015-07-27T02:34:23Z

(Also, the description of the class is Distributions over an n-dimensional unit hypersphere.)

tcstewar · 2015-07-27T02:38:19Z

Although, if that's the case, I would move to rename UniformHypersphere to UniformUnitHypersphere.

I would vote against that renaming, just on backwards compatibility reasons. :)

xchoo · 2015-07-27T02:39:00Z

I would vote against that renaming, just on backwards compatibility reasons. :)

I guessssssssssss.........

jgosmann · 2015-07-27T02:43:21Z

You could allow a number as argument for surface (in addition to a boolean) which would then be treated as high.

xchoo · 2015-07-27T02:45:37Z

But how would you indicate if you want to have the points on the surface or not? E.g., you want a hypersphere of radius 2, and all points on the surface.

jgosmann · 2015-07-27T03:13:07Z

low=2, surface=2'? (but this is somewhat inconsistent withrandintwherelow == high` is not allowed)

Maybe we should implement two distributions: One samples from the whole hypersphere (with arguments low and high) and the other one just from the surface (by internally using the first distribution from the whole hypersphere and normalizing) with one argument radius. For backward compatibility we would have to keep the current API too, but we could mark it as deprecated and remove it one (or a few more releases) later.

tcstewar · 2015-07-27T03:48:53Z

Your documentation still doesn't indicate the interaction between surface and low. In particular, if surface is True, then low is ignored. Furthermore, your documentation still refers to the high parameter, which has been removed.

I will also note that, how it currently stands, setting surface to True gives exactly the same result as setting low to 1.0, and setting surface to False gives exactly the same result as setting low to 0.0. This suggests that you could also get rid of the low parameter and just have surface accept a float or a boolean. This would at least get rid of the confusion surrounding the interaction between the two variables.

xchoo · 2015-07-27T03:51:30Z

This suggests that you could also get rid of the low parameter and just have surface accept a float or a boolean. This would at least get rid of the confusion surrounding the interaction between the two variables.

But naming that parameter surface doesn't make any sense. The surface parameter name has been left there for compatibility. Ideally, I'd have a high and a low and no surface parameter.

tcstewar · 2015-07-27T03:52:23Z

One samples from the whole hypersphere (with arguments low and high) and the other one just from the surface (by internally using the first distribution from the whole hypersphere and normalizing) with one argument radius. For backward compatibility we would have to keep the current API too, but we could mark it as deprecated and remove it one (or a few more releases) later.

Hmm... I'd like to avoid bringing radius into this discussion, as that's an Ensemble-level parameter and I could see also sorts of potential for confusion here.

Also, before looking at an API change like that, I'd still like to know what the use case is for something like this. I've never wanted to sample from a uniform hyper-doughnut before -- why do we want that capability now?

xchoo · 2015-07-27T03:53:42Z

I do. Same logic as to why I use a Uniform distribution with a non-zero low when messing around with non standard intercepts.

xchoo · 2015-07-27T03:54:33Z

(i.e. I want to provide only valid sample points within the intercept ranges)

tcstewar · 2015-07-27T03:54:36Z

But naming that parameter surface doesn't make any sense. The surface parameter name has been left there for compatibility. Ideally, I'd have a high and a low and no surface parameter.

It makes as much sense to me as low and high do. I had to read your code in order to understand that it wasn't a bounding box, which was my initial assumption.

xchoo · 2015-07-27T03:56:06Z

It makes as much sense to me as low`` andhigh``` do. I had to read your code in order to understand that it wasn't a bounding box, which was my initial assumption.

I could use the name min_magnitude and max_magnitude. I used low and high to keep it kinda consistent with the uniform distribution.

hunse · 2015-12-11T20:28:28Z

nengo/dists.py

        Default: False

    """

-    def __init__(self, surface=False):
+    def __init__(self, min_magnitude=0, surface=False):


The new parameter should go after the existing surface, just in case people were using it positionally. Note that this change will affect the position in the docstring and in __repr__, too.

hunse · 2015-12-11T20:30:07Z

nengo/dists.py

    surface : bool
        Whether sample points should be distributed uniformly
        over the surface of the hyperphere (True),
        or within the hypersphere (False).
+        Note: Setting surface to True generates sample points from the
+        hypersphere surface regardless of what the value of min_magnitude is.


I'd remove this note, and in the description of min_magnitude say that it's ignored if surface == True.

hunse · 2015-12-11T20:33:21Z

nengo/dists.py

+        such that the sampled vector magnitudes >= min_magnitude.
+        The default value and minimum value for min_magnitude is 0.
+        The maximum value for min_magnitude is 1. Values outside this range
+        will be clipped to this range.


Can this be more concise? e.g. "Lower bound on the returned vector magnitudes (i.e. magnitude >= min_magnitude). Must be in the range [0, 1]; defaults to 0. Ignored if surface == True."

Seanny123 · 2015-12-11T21:12:59Z

@hunse I fixed the rest of your comments and it's now passing all the tests. Thanks for being so throrough! Do you think it's ready to merge now?

hunse · 2015-12-11T21:24:51Z

nengo/dists.py

-        return "UniformHypersphere(%s)" % (
-            "surface=True" if self.surface else "")
+        return "UniformHypersphere(min_magnitude=%s%r)" % (
+            ", surface=True" if self.surface else "", self.min_magnitude,)


This is still not right. Also, min_magnitude should only appear if it's different from the default.

Seanny123 · 2017-04-22T08:50:05Z

@hunse I did my best to fix your comments on this issue. Does it look okay now?

jgosmann · 2017-04-22T14:10:59Z

nengo/dists.py

+    min_magnitude : Number, optional (Default: 0)
+        Lower bound on the returned vector magnitudes (i.e.
+        magnitude >= min_magnitude). Must be in the range [0, 1]; defaults
+        to 0. Values outside this range will be clipped to this range. Ignored


I think this description is not entirely correct. If values outside of the range get clipped, I understand that values get sampled in the range [0, 1] and then everything below min_magnitude will set to min_magnitude (which will cluster vectors at min_magnitude). But as I understand the code values are sampled from [min_magnitude, 1] and thus are without clustering at min_magnitude.

I think all this docstring is saying is that values for min_magnitude will be clipped to [0, 1]. I agree that we should have an exception instead of clipping. This is already done by the low and high in the parameter definition.

jgosmann · 2017-04-22T14:12:26Z

nengo/dists.py

        super(UniformHypersphere, self).__init__()
+        self.min_magnitude = np.clip(min_magnitude, 0, 1)


I would prefer to raise an exception when min_magnitude is outside of the range (which should happen by just removing the call to clip because it should be checked by the NumberParam).

I agree, and I think this is what caused @jgosmann's confusion with the docstring.

hunse · 2017-04-22T15:47:08Z

nengo/dists.py

    """

    surface = BoolParam('surface')
+    min_magnitude = NumberParam('min_magnitude', low=0, high=1)


Add high_open=True so that we don't allow min_magnitude == 1, which I think would cause problems because then we'd be sampling from an empty interval.

hunse · 2017-04-22T16:17:20Z

Ok, I've addressed @jgosmann's suggestions.

jgosmann

Thank you, @hunse!

Seanny123 · 2017-04-23T04:27:54Z

Those changes look awesome. Thanks @hunse. I wasn't sure how much of the tests were redundant and how to fix the __repr__ string code.

tbekolay

This LGTM; I pushed a fixup to fix the changelog formatting (can't have spaces after / before the asterisks).

I share the concerns people have about the surface and min_magnitude parameters being slightly redundant... Unfortunately I think there's no way around this, since surface=True is more clear than min_magntiude=1. However, to at least be explicit when the two parameters are both set, I added warning when surface is True and min_magnitude > 0 in the fixup commit.

Will fix up history and merge when CI finishes, unless someone stops me in the next half hour or so!

xchoo added the needs review label Jul 26, 2015

arvoelke reviewed Jul 26, 2015
View reviewed changes

tcstewar reviewed Jul 26, 2015
View reviewed changes

hunse reviewed Dec 11, 2015
View reviewed changes

Seanny123 changed the title ~~Added low & high options to UniformHypersphere dist~~ Added lower bound option to UniformHypersphere dist Dec 11, 2015

hunse reviewed Dec 11, 2015
View reviewed changes

Seanny123 force-pushed the UniformHypersphereLowHighDist branch from 739daa7 to f18d8b4 Compare December 11, 2015 20:32

hunse reviewed Dec 11, 2015
View reviewed changes

Seanny123 removed the needs changes label Dec 11, 2015

hunse reviewed Dec 11, 2015
View reviewed changes

drasmuss added the needs changes label Jun 27, 2016

jgosmann added the needs rebase label Aug 24, 2016

tbekolay removed the needs changes label Apr 18, 2017

Seanny123 force-pushed the UniformHypersphereLowHighDist branch from 7344746 to 5176c6f Compare April 22, 2017 08:42

Seanny123 removed the needs rebase label Apr 22, 2017

Seanny123 force-pushed the UniformHypersphereLowHighDist branch from 224bf9a to 17ee763 Compare April 22, 2017 10:23

jgosmann requested changes Apr 22, 2017

View reviewed changes

hunse reviewed Apr 22, 2017

View reviewed changes

jgosmann approved these changes Apr 22, 2017

View reviewed changes

tbekolay approved these changes May 11, 2017

View reviewed changes

tbekolay force-pushed the UniformHypersphereLowHighDist branch from 1847d12 to 88ce22e Compare May 11, 2017 19:25

Added min_magnitude to UniformHypersphere

07c09ed

tbekolay force-pushed the UniformHypersphereLowHighDist branch from 88ce22e to 07c09ed Compare May 11, 2017 21:10

tbekolay merged commit 07c09ed into master May 11, 2017

tbekolay deleted the UniformHypersphereLowHighDist branch May 11, 2017 21:11

		super(UniformHypersphere, self).__init__()
		self.min_magnitude = np.clip(min_magnitude, 0, 1)

Added lower bound option to UniformHypersphere dist #799

Added lower bound option to UniformHypersphere dist #799

Conversation

xchoo commented Jul 26, 2015

Choose a reason for hiding this comment

arvoelke commented Jul 26, 2015

xchoo commented Jul 26, 2015

jgosmann commented Jul 26, 2015

arvoelke commented Jul 26, 2015

xchoo commented Jul 26, 2015

tcstewar commented Jul 26, 2015

xchoo commented Jul 26, 2015

xchoo commented Jul 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgosmann commented Jul 27, 2015

tcstewar commented Jul 27, 2015

xchoo commented Jul 27, 2015

jgosmann commented Jul 27, 2015

xchoo commented Jul 27, 2015

xchoo commented Jul 27, 2015

tcstewar commented Jul 27, 2015

xchoo commented Jul 27, 2015

jgosmann commented Jul 27, 2015

xchoo commented Jul 27, 2015

jgosmann commented Jul 27, 2015

tcstewar commented Jul 27, 2015

xchoo commented Jul 27, 2015

tcstewar commented Jul 27, 2015

xchoo commented Jul 27, 2015

xchoo commented Jul 27, 2015

tcstewar commented Jul 27, 2015

xchoo commented Jul 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Seanny123 commented Dec 11, 2015

Choose a reason for hiding this comment

Seanny123 commented Apr 22, 2017

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

hunse commented Apr 22, 2017

jgosmann left a comment

Choose a reason for hiding this comment

Seanny123 commented Apr 23, 2017

tbekolay left a comment

Choose a reason for hiding this comment