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

[R] improve more docstrings #9919

Merged
merged 9 commits into from
Dec 26, 2023
Merged

[R] improve more docstrings #9919

merged 9 commits into from
Dec 26, 2023

Conversation

mayer79
Copy link
Contributor

@mayer79 mayer79 commented Dec 23, 2023

This PR updates the docstrings of these (plotting) functions:

  • xgb.ggplot.R
  • xgb.importance.R
  • xgb.model.dt.tree.R
  • xgb.plot.deepness.R
  • xgb.plot.importance.R
  • xgb.plot.multi.trees.R
  • xgb.plot.shap.R
  • xgb.plot.tree.R

As such, it is the next step towards #9875.

Like in #9906, the changes include:

  1. Switch to Markdown style in Roxygen
  2. Fix inconsistencies
  3. Improve language

There are no changes in the code itself.

Note there is one inconsistency that I have not fixed because I don't know what is better: An object of class "xgb.Booster" or an object of class xgb.Booster. I.e., how to highlight classes.

@trivialfis
Copy link
Member

An object of class "xgb.Booster" or an object of class xgb.Booster. I.e., how to highlight classes.

The latter seems to fit markdown better?

@mayer79
Copy link
Contributor Author

mayer79 commented Dec 24, 2023

That's a point.

"ggplot" e.g. doesn't use any highlighting:

  • #' ggplot() initializes a ggplot object.
  • #' @param object ggplot2 object to summarise

Similarly, {lubridate}:

  • #' Interval class objects have two slots:
  • #' @param x Object to be coerced to a duration

{dplyr} seems to avoid using class names, but here is an example that also uses no highlighting:

  • #' @param .data A data frame, data frame extension (e.g. a tibble), or a
    #' lazy data frame (e.g. from dbplyr or dtplyr). See Methods, below, for
    #' more details.

But there are also snippets in the Tidyverse docu that use backticks.

Hmm 😀. It is not super important, I just wanted to note it somewhere.

@trivialfis
Copy link
Member

I hope the GitHub user param is okay with the ping. ;-)

I don't have a strong preference. For my personal choice, I would simply follow the tidyverse's convention.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Thank you for the nice work on the document! I don't have a strong preference regarding the style of reference and quote, but would be great if we could pick one and use it consistently. I left some questions in the comments for various uses.

#' @param plot (base R barplot) whether a barplot should be produced.
#' If FALSE, only a data.table is returned.
#' @param n_clusters (ggplot only) a \code{numeric} vector containing the min and the max range
#' @param importance_matrix A `data.table` as returned by [xgb.importance()].
Copy link
Member

Choose a reason for hiding this comment

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

Trying to find the pattern of reference, what's the square bracket? I see that you are using single quote, plain text, square bracket in a mix for various internal references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is like \link{\code{xgb.importance}}, https://roxygen2.r-lib.org/articles/markdown.html (middle section).

#' @param plot_height the height of the diagram in pixels.
#' @param render a logical flag for whether the graph should be rendered (see Value).
#' @param feature_names Character vector used to overwrite the feature names
#' of the model. The default (`NULL`) uses the original feature names.
Copy link
Member

Choose a reason for hiding this comment

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

Is NULL or (NULL)?

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 preference.

@trivialfis
Copy link
Member

Let's merge it to unblock the progress. ;-) Again, thank you for the nice work on the documents.

Note there is one inconsistency that I have not fixed because I don't know what is better: An object of class "xgb.Booster" or an object of class xgb.Booster. I.e., how to highlight classes.

For future references, let's stick to backticks.

@trivialfis trivialfis merged commit 52620fd into dmlc:master Dec 26, 2023
@mayer79
Copy link
Contributor Author

mayer79 commented Dec 26, 2023

Thanks! Backticks are fine 😀

@mayer79 mayer79 mentioned this pull request Aug 26, 2024
25 tasks
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.

2 participants