Added temperature flag to generation script #131

robinsloan · 2016-10-08T22:25:03Z

It's nice to be able to specify sampling "temperature" when generating output, usually for aesthetic reasons, so I added some code to scale the sampling probabilities if a temperature other than 1.0 is provided.

Demo: https://soundcloud.com/robinsloan/sets/tensorflow-wavenet-temperature-demo

jyegerlehner · 2016-10-09T02:25:00Z

generate.py

+            np.seterr(divide='ignore')
+            prediction = np.log(prediction) / args.temp
+            prediction[np.isneginf(prediction)] = 0
+            prediction = np.exp(prediction) / np.sum(np.exp(prediction))


I think this line is better implemented as something like:

prediction = prediction - scipy.misc.logsumexp(prediction) prediction = np.exp(prediction)

By operating in the log domain as much as possible we can avoid the division which leads to instability when we divide by something very close to zero. Especially if we ever want to run using float16. And you were in the log domain with prediction already, anyway.

Never would have thought of that; thanks for the guidance!

robinsloan · 2016-10-10T16:41:56Z

I made @jyegerlehner's suggested changes, which introduces a scipy dependency, which is… not great? But there's no direct numpy equivalent to the scipy.misc.logsumexp function, unfortunately.

jyegerlehner · 2016-10-10T17:02:40Z

If we don't like the dependency, would logsumexp() be equivalent to np.log(np.sum(np.exp(prediction)))?

I wouldn't uncritically paste my code in there; I was just describing an idea, and my code is not usually right the first time around :).

A test that shows that the temperature sampling with T=1 produces same distribution as without would be good.

And perhaps, if I understand this, in the limit as T->0, it should be equivalent to choosing the most likely quantization level. And then as T-> inf, all quantization levels become equally likely?

robinsloan · 2016-10-10T17:09:21Z

Great idea on the test. I've never written a Python test before so I'll poke at the existing tests in this project and figure it out.

I'll try that numpy approach. I tried something different (no.logaddexp.reduce) and didn't get what I expected so I think a more stepwise approach would would be better for me.

ibab · 2016-10-10T17:18:50Z

I'm fine with using scipy as long as we put it into the requirements.txt :)
Note that logsumexp(predictions) is different from doing log(sum(exp(predictions))).
It shifts by max(predictions) in order to avoid underflow.

This is a really nice addition to the project!

lemonzi

A couple nits.

lemonzi · 2016-10-12T18:57:20Z

generate.py

@@ -179,6 +187,15 @@ def main():

        # Run the WaveNet to predict the next sample.
        prediction = sess.run(outputs, feed_dict={samples: window})[0]
+
+        # Scale sample distribution using temperature, if applicable.
+        if (args.temp != 1.0 and args.temp > 0):


Parentheses are not needed here.

lemonzi · 2016-10-12T18:57:51Z

generate.py

@@ -36,6 +39,11 @@ def _str_to_bool(s):
        default=SAMPLES,
        help='How many waveform samples to generate')
    parser.add_argument(
+        '--temp',


Please use temperature or sampling_temperature.

robinsloan · 2016-10-12T21:29:44Z

Quick question for @jyegerlehner or anyone else as I'm fixing this up:

When I scale the prediction distribution with temperature=1.0, running it through log space, I get back the original distribution, as expected, though it's not identical -- it is np.allclose to within 1e-09. Does that sound reasonable? (I have no intuition for this.)

jyegerlehner · 2016-10-13T00:58:33Z

@robinsloan That sounds plenty good to me. We might want to future-proof for fp16, and make the tolerance wider, because float16 only has 6-7 significant digits. Maybe use 1e-4 for a tolerance?

[Edit] oops, float32 has 6-7 digits. fp16 only has 3 or 4.

robinsloan · 2016-10-13T02:26:06Z

Updates:

Application of temperature to prediction distribution now happens in log space
A test ensures scaling at temperature=1.0 gives us back the original predictions, to within 1e-5
No scipy dependency
Style changes, per @lemonzi

Thanks for all the feedback & support on this!

lemonzi · 2016-10-13T18:43:24Z

generate.py

@@ -7,12 +7,14 @@
 import os

 import librosa
+


Why this blank line?

lemonzi · 2016-10-13T18:44:41Z

generate.py

+        np.seterr(divide='warn')
+
+        # Prediction distribution at temperature=1.0 should be unchanged after scaling.
+        if args.temperature == 1.0:


This should be a unit test, we can move it later when tests for generation are ready.

So, when #142 is merged.

I haven't written test coverage in Python before and couldn't determine where in the codebase to put this, if not here. Pointers appreciated!

You didn't find it because it doesn't exist yet -- there is a PR that provides tests for this file. Thanks for specifying how to test it, though! We'll move it later, no problem.

lemonzi · 2016-10-13T18:45:39Z

generate.py

@@ -27,6 +29,12 @@ def _str_to_bool(s):
                             'boolean, got {}'.format(s))
        return {'true': True, 'false': False}[s.lower()]

+    def _ensure_positive_float(f):


Shouldn't this be generic and not specific to the sampling temperature?

I didn't want to change anything outside the scope of this feature; maybe this suggestion is better left to a general refactoring of the argument parsing?

Ohhh wait I see what you're saying. I didn't notice that the ArgumentTypeError indicated automatically the argument to which it was objecting. So the error can just say "yo you need a positive float." Got it got it.

Didn't you add this function as part of the feature? It has a very generic name but it's specific to the temperature parameter, that's why I mentioned this. I think argparse takes care of referring to the argument name when displaying an error, so I suggested leaving the "positive float" generic rather than adding a _parse_temperature function. The _str_to_bool is due for a refactor anyway though, so we could do both of them together later on, but I feel it's a but unnecessary.

Haha yep, we typed at the same time. Thanks!

lemonzi · 2016-10-13T18:46:55Z

For the other reviewers: these are minor nits, feel free to merge and mark to fix later.

lemonzi · 2016-10-14T18:56:10Z

LGTM

jyegerlehner · 2016-10-14T19:09:52Z

@lemonzi Would we want to wait to merge this until after PR 142 is merged? Those tests in 142 will be running this code, so we will get to see that they still pass before we merge this change.

ibab · 2016-10-14T19:11:03Z

LGTM, too!
Feel free to merge.

lemonzi · 2016-10-14T19:16:13Z

@jyegerlehner Good point, we can wait, there's only that conflict left to resolve.

ibab · 2016-10-14T19:22:17Z

@jyegerlehner: Okay, that's a good idea. @robinsloan will have to rebase on top of master once we merge that PR so that travis will run your test.

jyegerlehner · 2016-10-17T19:45:14Z

@robinsloan Could you push a dummy change to your branch (or rebase it?), to trigger another run of the tests, so we can see if they pass, now that the generation test has been merged? Thx

lemonzi · 2016-10-17T20:10:06Z

(You can do that with git commit --allow-empty -m "Trigger Travis")

robinsloan · 2016-10-18T02:04:51Z

That's new to me—thanks @lemonzi!

robinsloan added 2 commits October 8, 2016 15:05

Add temperature flag to generation script.

9dc55ab

Don't allow negative temps

2587443

jyegerlehner reviewed Oct 9, 2016

View reviewed changes

robinsloan added 2 commits October 8, 2016 23:14

Usemore numerically stable method in temperature code

59e0aeb

Added divide-by-zero warning back, for aesthetics

8aee2e1

lemonzi reviewed Oct 12, 2016

View reviewed changes

Do temperature scaling in log space and add test at temperature=1.0

e5d55b2

jyegerlehner added the call for votes: merge? label Oct 13, 2016

lemonzi reviewed Oct 13, 2016

View reviewed changes

jyegerlehner removed the call for votes: merge? label Oct 13, 2016

Make argparse positive float check more general; delete rogue newline

79e2f86

Empty commit to trigger CI

1518b16

jyegerlehner merged commit 2606971 into ibab:master Oct 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added temperature flag to generation script #131

Added temperature flag to generation script #131

robinsloan commented Oct 8, 2016

jyegerlehner Oct 9, 2016 •

edited

robinsloan Oct 9, 2016

robinsloan commented Oct 10, 2016

jyegerlehner commented Oct 10, 2016

robinsloan commented Oct 10, 2016

ibab commented Oct 10, 2016

lemonzi left a comment

lemonzi Oct 12, 2016

lemonzi Oct 12, 2016

robinsloan commented Oct 12, 2016

jyegerlehner commented Oct 13, 2016 •

edited

robinsloan commented Oct 13, 2016

lemonzi Oct 13, 2016

lemonzi Oct 13, 2016

lemonzi Oct 13, 2016

robinsloan Oct 14, 2016

lemonzi Oct 14, 2016

lemonzi Oct 13, 2016

robinsloan Oct 14, 2016

robinsloan Oct 14, 2016

lemonzi Oct 14, 2016

lemonzi Oct 14, 2016

lemonzi commented Oct 13, 2016

lemonzi commented Oct 14, 2016

jyegerlehner commented Oct 14, 2016 •

edited

ibab commented Oct 14, 2016

lemonzi commented Oct 14, 2016

ibab commented Oct 14, 2016

jyegerlehner commented Oct 17, 2016 •

edited

lemonzi commented Oct 17, 2016

robinsloan commented Oct 18, 2016

Added temperature flag to generation script #131

Added temperature flag to generation script #131

Conversation

robinsloan commented Oct 8, 2016

jyegerlehner Oct 9, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robinsloan commented Oct 10, 2016

jyegerlehner commented Oct 10, 2016

robinsloan commented Oct 10, 2016

ibab commented Oct 10, 2016

lemonzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robinsloan commented Oct 12, 2016

jyegerlehner commented Oct 13, 2016 • edited

robinsloan commented Oct 13, 2016

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

lemonzi commented Oct 13, 2016

lemonzi commented Oct 14, 2016

jyegerlehner commented Oct 14, 2016 • edited

ibab commented Oct 14, 2016

lemonzi commented Oct 14, 2016

ibab commented Oct 14, 2016

jyegerlehner commented Oct 17, 2016 • edited

lemonzi commented Oct 17, 2016

robinsloan commented Oct 18, 2016

jyegerlehner Oct 9, 2016 •

edited

jyegerlehner commented Oct 13, 2016 •

edited

jyegerlehner commented Oct 14, 2016 •

edited

jyegerlehner commented Oct 17, 2016 •

edited