Skip to content
This repository was archived by the owner on Jan 15, 2024. It is now read-only.

[BUGFIX] Remove test set from validation#1228

Merged
eric-haibin-lin merged 3 commits intodmlc:masterfrom
avinashsai:master
May 31, 2020
Merged

[BUGFIX] Remove test set from validation#1228
eric-haibin-lin merged 3 commits intodmlc:masterfrom
avinashsai:master

Conversation

@avinashsai
Copy link
Member

Fixes #372

@avinashsai avinashsai requested a review from a team as a code owner May 8, 2020 17:53
@mli
Copy link
Member

mli commented May 8, 2020

Job PR-1228/1 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-1228/1/index.html

@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #1228 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1228   +/-   ##
=======================================
  Coverage   87.45%   87.45%           
=======================================
  Files          81       81           
  Lines        7365     7365           
=======================================
  Hits         6441     6441           
  Misses        924      924           

@avinashsai
Copy link
Member Author

@leezu @szha Please review this

@avinashsai avinashsai requested a review from leezu May 11, 2020 18:57
Copy link
Member

@eric-haibin-lin eric-haibin-lin left a comment

Choose a reason for hiding this comment

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

We still need the functionality to get test score for GNMT. For transformer, we have that in the infer_transformer.py. Can we add an option in train_gnmt that only does inference on the test set?

@avinashsai
Copy link
Member Author

Similarly, an argument should be added in train_transformer.py whether to perform inference on test set? It should be default set to true?

Copy link
Member

@eric-haibin-lin eric-haibin-lin left a comment

Choose a reason for hiding this comment

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

No, no need to add that option to train_transformer.py since we already have a dedicated script for transformer inference

@avinashsai
Copy link
Member Author

@StrayBird-ATSH GluonNLP-py3-master-gpu-doc is failing.

@chenw23
Copy link
Member

chenw23 commented May 15, 2020

Thanks for reminding. I have noticed it on some other pull request. It might be due to some global internet connectivity failures. We are sorting out reasons.

@avinashsai
Copy link
Member Author

@StrayBird-ATSH Any updated on this?

@avinashsai
Copy link
Member Author

@StrayBird-ATSH can you restart builds to see if the issue has been resolved?

@chenw23
Copy link
Member

chenw23 commented May 23, 2020

OK. I will have a look into it.

@avinashsai
Copy link
Member Author

@StrayBird-ATSH I guess the error is fixed. If possible, can you restart builds?

@chenw23
Copy link
Member

chenw23 commented May 30, 2020

@StrayBird-ATSH I guess the error is fixed. If possible, can you restart builds?

Yes, would you please merge the latest master branch into your pull request, especially to include #1236 ?
Thanks!

@mli
Copy link
Member

mli commented May 31, 2020

Job PR-1228/10 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-1228/10/index.html

@avinashsai
Copy link
Member Author

@eric-haibin-lin please review this

@eric-haibin-lin eric-haibin-lin merged commit 4d1c087 into dmlc:master May 31, 2020
@eric-haibin-lin
Copy link
Member

thanks for the contribution!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants