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

Pi output in tensorflow is incorrect: missing softmax layer. #214

Open
josey4869 opened this issue Sep 7, 2020 · 4 comments
Open

Pi output in tensorflow is incorrect: missing softmax layer. #214

josey4869 opened this issue Sep 7, 2020 · 4 comments

Comments

@josey4869
Copy link

josey4869 commented Sep 7, 2020

In othello.tensorflow.OthelloNNet.py:

The pi output of the model is

self.pi = Dense(s_fc2, self.action_size) (line 36)

which is incorrect. In fact, it should be

self.pi = tf.nn.softmax(Dense(s_fc2, self.action_size)).

By contrast, the implementation in Keras is correct. In othello.keras.OthelloNNet.py:

self.pi = Dense(self.action_size, activation='softmax', name='pi')(s_fc2) (line 28).

Pull request: #215 (comment)

@goshawk22
Copy link
Contributor

The softmax is applied in the line after and the layer is assigned to the variable self.prob which is the used instead of self.pi in the Nnet file.

@josey4869
Copy link
Author

The softmax is applied in the line after and the layer is assigned to the variable self.prob which is the used instead of self.pi in the Nnet file.

Thanks for the reply! According to line 48 & line 131:

self.loss_pi = tf.losses.softmax_cross_entropy(self.target_pis, self.pi),

It seems self.pi instead of self.prod is used for computing pi_loss? So I guess we should either assign a softmax layer before outputting self.pi or feed self.prod instead of self.pi into pi_loss?

@goshawk22
Copy link
Contributor

Yes I think it might make more sense to just change it to add the softmax layer to self.pi to avoid confusion.

@rlronan
Copy link
Contributor

rlronan commented Jun 9, 2021

Softmax crossentropy expects logits in tensorflow, so softmax should not be applied to pi directly.
https://www.tensorflow.org/api_docs/python/tf/compat/v1/losses/softmax_cross_entropy

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

No branches or pull requests

3 participants