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

fix bug in ch3/linear_regression_tf.py #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

discoverkl
Copy link

@discoverkl discoverkl commented Apr 19, 2018

I think there may be a bug due to brordcasting rule. Please take a look.

@rbharath
Copy link
Collaborator

Thanks for the PR! I think you might be right about the broadcasting error... I'll try rerunning this code with the fix to double check on my end.

@RAvontuur
Copy link

With the above fix, the system converges to the right solution.
After setting the noise to zero, and initialize the system with w=5 and b=2, the loss is now zero (as expected), and not some high positive value as it was before the fix.

Because of this, the following explanation in the book is not correct and requires an update:
'What happened on this system? Why didn’t TensorFlow learn the correct function despite being trained to convergence? This example provides a good illustration of one of the weaknesses of gradient descent algorithms. There is no guarantee of finding the true solution! The gradient descent algorithm can get trapped in local minima. That is, it can find solutions that look good, but are not in fact the lowest minima of the loss function'

Please, add a correct update of this explanation as a comment to the code. This makes it better understandable for future readers.

@rbharath
Copy link
Collaborator

Thanks for the feedback here. We'll make sure to fix this bug in a future printing of the book.

As a quick note, the explanation isn't wrong though. It's entirely common to see instability in training more complex models. It turns our the behavior on this linear system is in fact stable after this bugfix, but there are a number of unstable nonlinear systems you will encounter in practice. We will add a note to explain this.

@hamelsmu
Copy link
Contributor

hamelsmu commented Jul 18, 2018

You can also fix this bug by merging #17

I agree this is confusing / misleading for readers. When reading the book, I was very skeptical of this model not converging and set out to debug the model. I noticed if you keep training the model the learned slope goes to zero (a flat line), which I found quite odd and gave me intuition that somehow the loss function was ill defined, because I noticed the loss decreasing even though the visualization of the learned model kept looking worse.

One idea is you could demonstrate how to use tf eager on how to debug this situation in the book, which is a useful thing to learn.

cc: @hohsiangwu @ankushagarwal

@rbharath
Copy link
Collaborator

@hamelsmu Good suggestion! We will add a section on debugging this model in the next edition of this book. Our apologies again for letting this error slip through review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants