Skip to content

Conversation

@WindQAQ
Copy link
Member

@WindQAQ WindQAQ commented Jun 17, 2019

Closes #295.

@Squadrick
Copy link
Member

The docstring says: y_true: 1-D integer Tensor with shape [batch_size] of multiclass integer labels. The old assertion took care of that, with the new assert, we can pass in a higher dimensional Tensor, and it'll pass that assert.

Take for example y_true passed as a 3-D Tensor.

yt_shape = tf.shape(y_true) # Tensor("Shape:0", shape=(3,), dtype=int32)
yt_shape.shape # (3,)
yt_shape.shape.rank # 1, passes the new assert

The older assert seemed to do as intended, however during compile Keras passes in y_true as the following:

Tensor("y_true:0", shape=(None, None), dtype=float32)

This is what causes the older assert to fail.

@WindQAQ
Copy link
Member Author

WindQAQ commented Jun 17, 2019

@Squadrick, you are right. I made some mistakes on shape inference here...

@WindQAQ
Copy link
Member Author

WindQAQ commented Jun 17, 2019

I think it would be a dummy check as there is not explicit shape or rank checking in tf.keras.losses. @facaiy and @Squadrick, what do you feel about this?

@facaiy
Copy link
Member

facaiy commented Jun 17, 2019

labels = tf.reshape(labels, [lshape[0], 1]) will fail for invalid shape, so I think it safe to remove the check.

@WindQAQ
Copy link
Member Author

WindQAQ commented Jun 17, 2019

Facai, I add a testcase for invalid shape with calling triplet_semihard_loss directly. (Actually, I do not know how to specify the shape of y_true for keras sequential model...)

@facaiy
Copy link
Member

facaiy commented Jun 17, 2019

I delete the requirement for new test case, Tzu-Wei. Let's add it later if we think it necessary :-) Apologized for the misleading message.

@facaiy
Copy link
Member

facaiy commented Jun 17, 2019

@Squadrick Hi, Dheeraj, what do you think?

@Squadrick
Copy link
Member

Yeah, I think it's best we remove the check for now. Taking a look at tf.keras.losses, they don't seem to do a shape check either. As long as the results are numerically accurate, it should be good to go.

@WindQAQ
Copy link
Member Author

WindQAQ commented Jun 17, 2019

I delete the requirement for new test case, Tzu-Wei. Let's add it later if we think it necessary :-) Apologized for the misleading message.

No worries :-) It makes sense to add it when necessary.

@WindQAQ WindQAQ merged commit 229344f into tensorflow:master Jun 17, 2019
@WindQAQ WindQAQ deleted the triplet-loss-model-compile branch June 17, 2019 13:02
@hhhwwwuuu
Copy link

ummm. so, how should I fix ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to compile model using triplet loss

5 participants