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

Replace cython sampler with mxnet sampler to improve user experience #270

Merged
merged 10 commits into from
Aug 13, 2018

Conversation

eric-haibin-lin
Copy link
Member

@eric-haibin-lin eric-haibin-lin commented Aug 12, 2018

Description

Now we have _sample_unique_zipfian in mxnet, we don't need to build with cython.
Ran the training script end2end for 33 epochs, reached test ppl 44.15.

Checklist

Essentials

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@szha szha mentioned this pull request Aug 12, 2018
20 tasks
@szha szha added the release focus Progress focus for release label Aug 12, 2018
@eric-haibin-lin eric-haibin-lin changed the title [WIP] replace cython sampler with mxnet sampler to improve user experience Replace cython sampler with mxnet sampler to improve user experience Aug 13, 2018
@codecov
Copy link

codecov bot commented Aug 13, 2018

Codecov Report

Merging #270 into master will decrease coverage by 5.32%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
- Coverage   75.12%   69.79%   -5.33%     
==========================================
  Files          74       75       +1     
  Lines        6202     6277      +75     
  Branches      964      988      +24     
==========================================
- Hits         4659     4381     -278     
- Misses       1319     1697     +378     
+ Partials      224      199      -25
Impacted Files Coverage Δ
scripts/tests/test_scripts.py 96% <100%> (-4%) ⬇️
scripts/nmt/train_transformer.py 0% <0%> (-73.95%) ⬇️
gluonnlp/data/dataloader.py 26.08% <0%> (-69.57%) ⬇️
scripts/nmt/loss.py 42.37% <0%> (-18.65%) ⬇️
scripts/nmt/transformer.py 84.67% <0%> (-6.52%) ⬇️
scripts/nmt/translation.py 82.05% <0%> (-5.13%) ⬇️
scripts/nmt/utils.py 75% <0%> (-3.58%) ⬇️
scripts/tests/test_sampler.py 100% <0%> (ø)
...cripts/language_model/large_word_language_model.py 0.41% <0%> (+0.41%) ⬆️
scripts/language_model/sampler.py 75.55% <0%> (+75.55%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07b0edd...e4ca873. Read the comment docs.

@mli
Copy link
Member

mli commented Aug 13, 2018

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

@szha szha merged commit 2bf8adc into dmlc:master Aug 13, 2018
@eric-haibin-lin eric-haibin-lin deleted the sampler branch September 12, 2018 05:21
paperplanet pushed a commit to paperplanet/gluon-nlp that referenced this pull request Jun 9, 2019
…mlc#270)

* use mxnet op instead of ctype op

* reduce verbosity in table

* update instruction

* fix lint

* update mxnet dependency

* add test

* fix env var

* fix CI weird err

* update mxnet ver
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release focus Progress focus for release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants