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 TFConvBertModelIntegrationTest::test_inference_masked_lm Test #10104

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

abhishekkrthakur
Copy link
Member

No description provided.

Copy link
Contributor

@jplu jplu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot for fixing!!

@abhishekkrthakur abhishekkrthakur merged commit 480a9d6 into master Feb 9, 2021
@abhishekkrthakur abhishekkrthakur deleted the fix_test_convert branch February 9, 2021 19:22
@patrickvonplaten
Copy link
Contributor

@abhishekkrthakur - this doesn't look good to me.

Just changing the hardcoded integration test to values that make the test pass does not seem like the way to go here. The PyTorch integration test:

[[[-0.0348, -0.4686, -0.3064], [0.2264, -0.2699, -0.7423], [0.1032, -0.4501, -0.5828]]]
still has the old values and passes, which to me is an indicator that the TF implementation or the PyTorch implementation is not correct.

Also, it would be great if we could not merge PRs that have no description and that neither @sgugger, @LysandreJik or I approved.

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.

3 participants