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

[BUGFIX] Corrected np.random.randint upper limit in data.stream.py #935

Merged
merged 2 commits into from
Sep 24, 2019

Conversation

ggbaro
Copy link
Contributor

@ggbaro ggbaro commented Sep 19, 2019

Description

Since default np.int_ is int32 for Windows, an upper limit for random seed of 2**32 rises a ValueError: high is out of bounds for int32.
with np.iinfo(np.int_).max, np.random.randint is platform-independent

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • 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

  • very small fix to allow Windows users to use module (otherwise random seed is bugged)

cc @dmlc/gluon-nlp-team

Since default np.int_  is int32 for Windows, an upper limit for random seed of 2**32 rises a `ValueError: high is out of bounds for int32`.
with `np.iinfo(np.int_).max`, np.random.randint is platform-independent
@ggbaro ggbaro requested a review from a team as a code owner September 19, 2019 23:13
@codecov
Copy link

codecov bot commented Sep 19, 2019

Codecov Report

Merging #935 into master will increase coverage by 1.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #935     +/-   ##
=========================================
+ Coverage   89.51%   90.71%   +1.2%     
=========================================
  Files          67       67             
  Lines        6360     6389     +29     
=========================================
+ Hits         5693     5796    +103     
+ Misses        667      593     -74
Impacted Files Coverage Δ
src/gluonnlp/data/stream.py 84.97% <100%> (ø) ⬆️
src/gluonnlp/model/train/language_model.py 95.33% <0%> (-1.83%) ⬇️
src/gluonnlp/model/language_model.py 98.54% <0%> (-1.46%) ⬇️
src/gluonnlp/model/train/cache.py 97.82% <0%> (+0.2%) ⬆️
src/gluonnlp/model/attention_cell.py 94.55% <0%> (+1.36%) ⬆️
src/gluonnlp/model/transformer.py 91.2% <0%> (+1.5%) ⬆️
src/gluonnlp/model/block.py 53.19% <0%> (+2.12%) ⬆️
src/gluonnlp/data/utils.py 76.33% <0%> (+2.29%) ⬆️
src/gluonnlp/model/sequence_sampler.py 91.63% <0%> (+17.07%) ⬆️
src/gluonnlp/model/seq2seq_encoder_decoder.py 75% <0%> (+29.68%) ⬆️

@mli
Copy link
Member

mli commented Sep 19, 2019

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

@szha
Copy link
Member

szha commented Sep 23, 2019

It seems that the test_dataloader test is consistently hanging in the CI. For example: http://ci.mxnet.io/blue/organizations/jenkins/GluonNLP-py3-cpu-unittest/detail/PR-935/3/pipeline#step-159-log-1116

@leezu
Copy link
Contributor

leezu commented Sep 24, 2019

Could be due to using 64 bit instead of 32 bit range used before. I added a commit so that np.int32 is used

@leezu
Copy link
Contributor

leezu commented Sep 24, 2019

np.int_ can be 64 bit, and it is 64 bit on our CI

@mli
Copy link
Member

mli commented Sep 24, 2019

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

@leezu leezu merged commit e3f6dd0 into dmlc:master Sep 24, 2019
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.

6 participants