Skip to content
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

Fix issue #380 #381

Merged
merged 9 commits into from
Nov 16, 2018
Merged

Fix issue #380 #381

merged 9 commits into from
Nov 16, 2018

Conversation

const-ae
Copy link
Contributor

  • Reset .Random.seed after impute::impute.knn is called

- Reset .Random.seed after impute::impute.knn is called
@lgatto
Copy link
Owner

lgatto commented Nov 15, 2018

I think you are missing a comma before the curly bracket

test_that("seed is not set by knn imputation method" {

But well done for adding a unit test!

@const-ae
Copy link
Contributor Author

Oh yeah, you are right. I fixed the issue in my fork. Do I have to make a new pull request or does Github automatically see the new commit?

@codecov-io
Copy link

codecov-io commented Nov 15, 2018

Codecov Report

Merging #381 into master will increase coverage by <.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   71.36%   71.36%   +<.01%     
==========================================
  Files          82       82              
  Lines        8280     8284       +4     
==========================================
+ Hits         5909     5912       +3     
- Misses       2371     2372       +1
Impacted Files Coverage Δ
R/imputation.R 94.5% <80%> (-0.9%) ⬇️

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 69627bc...3f35996. Read the comment docs.

Copy link
Collaborator

@sgibb sgibb left a comment

Choose a reason for hiding this comment

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

@const-ae Great that you find such an error and created a PR. I have a few minor suggestions.

R/imputation.R Outdated Show resolved Hide resolved
R/imputation.R Outdated Show resolved Hide resolved
R/imputation.R Show resolved Hide resolved
sgibb and others added 3 commits November 15, 2018 17:42
Co-Authored-By: const-ae <const-ae@users.noreply.github.com>
Co-Authored-By: const-ae <const-ae@users.noreply.github.com>
Co-Authored-By: const-ae <const-ae@users.noreply.github.com>
Copy link
Collaborator

@sgibb sgibb left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Could you please add an entry in the NEWS.md file (with a short description of the problem, a reference to the issue/PR and your name)?

R/imputation.R Outdated Show resolved Hide resolved
R/imputation.R Outdated Show resolved Hide resolved
const-ae and others added 3 commits November 16, 2018 10:10
Co-Authored-By: const-ae <const-ae@users.noreply.github.com>
@const-ae
Copy link
Contributor Author

Thanks for helping me make a good PR.
I have updated the NEWS.md file, but do I need to do anything to generate the NEWS file or this done somewhere internally?

@sgibb sgibb merged commit 619a8f5 into lgatto:master Nov 16, 2018
@sgibb
Copy link
Collaborator

sgibb commented Nov 16, 2018

Great thanks! (The NEWS file is generated automatically by a .git/hook (which we all have locally, so if any one of us will create a commit the NEWS file will be updated, if you are interested in something like that please have a look at: maker setup-git-hooks)

@const-ae
Copy link
Contributor Author

Thanks, for the pointer. It looks like a quite useful project, but I have to admit I am still slightly intimidated by all the make stuff ;)

@lgatto
Copy link
Owner

lgatto commented Nov 16, 2018

Thank you both for the PR and the code review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants