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

fixed typos in R package docs #4345

Merged
merged 2 commits into from Apr 21, 2019
Merged

fixed typos in R package docs #4345

merged 2 commits into from Apr 21, 2019

Conversation

jameslamb
Copy link
Contributor

Thank you for this great project. I hope you'll consider this pull request to fix minor typos in the R-package documentation (found with devtools::spell_check()).

I apologize about the extra lines touched for trailing whitespace, that is a setting I have in my IDE. Please let me know if you would like me to revert those whitespace-only edits.

I also regenerated the documentation with devtools:;document() so the .Rd files in man/ would be consistent with the roxygen changes I made.

@jameslamb
Copy link
Contributor Author

I see that my build is failing with this error:

image

This is an error I saw a lot in my own projects (private and open source) when devtools went to 2.0.0.

This is a result of the use of craigcitro/r-travis in this project's CI. That project has been deprecated since 2016.

Do you have an issue open to replace it? I would love to contribute a fix!

@hcho3
Copy link
Collaborator

hcho3 commented Apr 9, 2019

@jameslamb That would be great! There's no open issue yet, but feel free to open a new pull request

@jameslamb
Copy link
Contributor Author

@hcho3 I opened #4348 to document this. Could you assign it to me? I could do it in the next day or two and submit a PR, if that is alright.

@trivialfis
Copy link
Member

trivialfis commented Apr 9, 2019

@jameslamb Thanks, please go ahead and submit a PR. ;-)

@jameslamb
Copy link
Contributor Author

I've rebased this to master now that #4353 has been merged. Should build successfully now.

@jameslamb
Copy link
Contributor Author

I think Travis is failing because of that same transient failure in the python_test stage I saw on #4353 and others.

@codecov-io
Copy link

codecov-io commented Apr 14, 2019

Codecov Report

Merging #4345 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #4345     +/-   ##
=========================================
- Coverage   67.93%   67.83%   -0.1%     
=========================================
  Files         132      132             
  Lines       12239    12206     -33     
=========================================
- Hits         8314     8280     -34     
- Misses       3925     3926      +1
Impacted Files Coverage Δ
tests/cpp/helpers.cc 90.9% <0%> (-2.23%) ⬇️
src/objective/multiclass_obj.cu 92.63% <0%> (-1.06%) ⬇️
src/data/sparse_page_source.cc 85.08% <0%> (ø) ⬆️
src/data/data.cc 81.49% <0%> (ø) ⬆️
tests/cpp/predictor/test_cpu_predictor.cc 100% <0%> (ø) ⬆️
tests/cpp/helpers.h 100% <0%> (ø) ⬆️
tests/cpp/data/test_sparse_page_dmatrix.cc 100% <0%> (ø) ⬆️
src/tree/updater_colmaker.cc 39.76% <0%> (+0.11%) ⬆️

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 be7bc07...61f9730. Read the comment docs.

@trivialfis trivialfis self-requested a review April 14, 2019 05:44
@@ -210,7 +211,7 @@ dtest <- xgb.DMatrix(agaricus.test$data, label = agaricus.test$label)
watchlist <- list(train = dtrain, eval = dtest)

## A simple xgb.train example:
param <- list(max_depth = 2, eta = 1, verbosity = 0, nthread = 2,
param <- list(max_depth = 2, eta = 1, silent = 1, nthread = 2,
Copy link
Member

Choose a reason for hiding this comment

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

The silent parameter is deprecated, see https://xgboost.readthedocs.io/en/latest/parameter.html .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trivialfis oh this is interesting. I didn't actually touch this line directly. File changes in .Rd files were auto-generated when I ran devtools::document().

Maybe I should rebase and try again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhhhh I wonder if someone in a previous PR manually edited the .Rd but not the corresponding roxygen block in the R code? I'll fix that on this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok yes, this is what happened! I walked through the blame and found that this parameter was changed in xgb.train.Rd in #4267 but not the corresponding roxygen content in the R source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 25dbe3a

Copy link
Member

Choose a reason for hiding this comment

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

@jameslamb Thanks for fixing my mistake! I restarted the test and will merge once they are done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, happy to help!

@trivialfis trivialfis merged commit 5e97de6 into dmlc:master Apr 21, 2019
@trivialfis
Copy link
Member

Thanks!

CodingCat pushed a commit to CodingCat/xgboost that referenced this pull request Apr 22, 2019
* fixed typos in R package docs

* updated verbosity parameter in xgb.train docs
@lock lock bot locked as resolved and limited conversation to collaborators Jul 20, 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.

None yet

4 participants