-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update learning rate for #116 and other small error #117
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
=======================================
Coverage 87.96% 87.96%
=======================================
Files 262 262
Lines 15500 15500
=======================================
Hits 13634 13634
Misses 1866 1866 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we are ready to make this change yet. We need to come up with a plan for how we are going to roll this out first.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @johnml1135)
machine/jobs/nmt_engine_build_job.py
line 77 at r1 (raw file):
model_trainer.train(progress=phase_progress, check_canceled=check_canceled) model_trainer.save() train_corpus_size = parallel_corpus.count()
Why was this change necessary?
Previously, ddaspit (Damien Daspit) wrote…
I made a change for the word alignment - and made it incorrectly. This is to fix the code. I can make a separate PR for this if we are not ready for the learning rate changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @johnml1135)
machine/jobs/nmt_engine_build_job.py
line 77 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
I made a change for the word alignment - and made it incorrectly. This is to fix the code. I can make a separate PR for this if we are not ready for the learning rate changes.
Yes, go ahead and make a separate PR for now.
Previously, ddaspit (Damien Daspit) wrote…
Done. |
af0194c
to
df24881
Compare
@ddaspit - I believe we are finally ready to make this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)
machine/jobs/nmt_engine_build_job.py
line 77 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
Done.
If the model_trainer.stats.train_corpus_size
is not returning the correct value, it should be fixed in the trainer class.
df24881
to
439ca30
Compare
Previously, ddaspit (Damien Daspit) wrote…
I believe the issue may have been resolved elsewhere. Let's move it out of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)
#116
This change is