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

[BUGFIX] fix valid candidates issue #1323

Merged
merged 3 commits into from
Sep 1, 2020

Conversation

liuzh47
Copy link
Contributor

@liuzh47 liuzh47 commented Aug 28, 2020

Description

Fix the dynamic mask sampling strategy. Fix Issue #1321

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

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

cc @dmlc/gluon-nlp-team

@liuzh47 liuzh47 requested a review from a team as a code owner August 28, 2020 08:05
@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #1323 into master will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1323      +/-   ##
==========================================
+ Coverage   84.14%   84.23%   +0.08%     
==========================================
  Files          42       42              
  Lines        6397     6426      +29     
==========================================
+ Hits         5383     5413      +30     
+ Misses       1014     1013       -1     
Impacted Files Coverage Δ
src/gluonnlp/data/tokenizers.py 77.67% <0.00%> (-0.03%) ⬇️
src/gluonnlp/models/transformer.py 98.93% <0.00%> (-0.01%) ⬇️
setup.py 0.00% <0.00%> (ø)
src/gluonnlp/data/sampler.py 96.55% <0.00%> (+0.32%) ⬆️
src/gluonnlp/utils/misc.py 52.53% <0.00%> (+0.63%) ⬆️
src/gluonnlp/sequence_sampler.py 87.34% <0.00%> (+0.78%) ⬆️
conftest.py 85.29% <0.00%> (+1.16%) ⬆️

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 32e87d4...e843f35. Read the comment docs.

@sxjscience
Copy link
Member

@liuzh91 Would you try to fix this along with the other Gumbel-topk issue? (So there is no need for me to merge twice).

@liuzh47
Copy link
Contributor Author

liuzh47 commented Sep 1, 2020

@liuzh91 Would you try to fix this along with the other Gumbel-topk issue? (So there is no need for me to merge twice).

OK, I'll work on that

@sxjscience sxjscience merged commit 1bd85b6 into dmlc:master Sep 1, 2020
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.

2 participants