[R] Working on Roxygen documentation

[mayer79]

Step towards

#9875

After this, only a few R scripts are left for switching to the markdown style.

[trivialfis]

Not sure what's the implication of changing , to .. I know this is generated, but not familiar enough with R type system to understand the change.

[mayer79]

This pops up on my side since quite some months. I can easily undo this change.

[trivialfis]

I'm okay with the changes as they are generated. I am just curious about the implications. We run doc gen on the CI as well, not sure what's the cause of the change.

[trivialfis]

I will leave it to @david-cortes for detailed reviews.

[david-cortes]

Some comments:

• The change of exporting S3 methods as regular functions with dots in the name would introduce issues elsewhere, particularly when dealing with namespaces directly (I can think of incorrectly dispatched functions when using rpy2 for example). These are S3 methods and should be exported as such - not doing otherwise would be a bug.
• This will create hard-to-solve merge conflicts with other PRs like this: [R] Remove reshape argument in predict #10330
  Would be ideal if these changes could be done after that PR to avoid issues. Perhaps you could base it off the last commit from that branch and then we merge them in sequence.
• In-doc links like xgb.parameters in predict.xgb.Booster now show with (empty) parentheses in the link text. I personally would prefer them without those parentheses, so that they'd match with the name of the function and the .Rd file. If we take the base R docs as a reference, I see there are some cases where empty parentheses are added, like coef() in the docs for stats::vcov, but the parentheses are not added to the text that forms part of the link (they are plain text next to the link text).
• While it makes the files easier to inspect visually, I'm not sure it's actually an advantage to have all these docs in markdown mode:
  • We'll likely need to compose docs from different sections in the future (e.g. for parameters in xgboost()), and for some sections the docs are in non-markdown mode due to issues like this: [R] Docs for function arguments format second paragraph as code block #10329
    Would all types of docs play along if they are in different modes?
  • While the syntax is decidely more intuitive at first sight, I'm finding it extremely hard to find any information about how to do something with this new roxygen markdown style or how to address some problem. It's comparatively not as hard to look for something like "roxygen how to add named section" or "roxygen how to create link to function from different package" in a search engine, but there's hardly any results for roxygen's markdown style, and the docs from roxygen developers are not comprehensive to say the least. For example, how would one go around finding the syntax for mathematical equations for this markdown mode if we decide to add such types of docs later?

[mayer79]

@david-cortes : The file "xgb.DMatrix.R" with the unintended code block in its help file is one of the files I have not worked on yet. Thus, you should not expect it to look great. I am fixing these things on the fly.

As an illustration that such issues are easy to fix, see my latest commit in this PR:

5e57bfe

A good and compact description of rmarkdown in Roxygen

Latex formulas are still the same, e.g., like this:

#' @section Partial Dependence Functions: 
#' 
#' Let \eqn{F: R^p \to R} denote the prediction function that maps the 
#' \eqn{p}-dimensional feature vector \eqn{\mathbf{x} = (x_1, \dots, x_p)}
#' to its prediction. Furthermore, let 
#' \deqn{
#'   F_s(\mathbf{x}_s) = E_{\mathbf{x}_{\setminus s}}(F(\mathbf{x}_s, \mathbf{x}_{\setminus s}))
#' }
#' be the partial dependence function of \eqn{F} on the feature subset
#' \eqn{\mathbf{x}_s}, where \eqn{s \subseteq \{1, \dots, p\}}, as introduced in 
#' Friedman (2001).

[trivialfis]

I will look into #10330 first to avoid potential conflicts.

[terrytangyuan]

I think this will break existing behaviors

[hcho3]

This is part of the work on the new R interface: #9810

[david-cortes]

It shouldn't be related to the changes in other PRs. In the current master branch, triggering docs build with roxygen doesn't remove the S3 method exports.

Removing the S3 exports would indeed break things elsewhere.

[mayer79]

I have now added the roxygen tags @method print xgboost and @method print xgb.Booster. This brings the NAMESPACE file back to its original state.

[terrytangyuan]

Did we remove any explicit s3 export statements?

[mayer79]

I have added a commit that fixes the problem by adding the roxygen tags @method print xgboost and @method print xgb.Booster. The NAMESPACE file is now untouched.

[trivialfis]

Hi @mayer79 , I have merged @david-cortes 's PR, could you please help rebase this one?

[terrytangyuan]

Removing the S3 exports would indeed break things elsewhere.

Great. Glad that we caught this earlier in the PR.

[trivialfis]

Overall looks good to me, mostly reformating and reording the existing document, please let me know if there's anything specific that I should pay closer attention.

I will merge the PR if there's no further comment from @david-cortes .

[mayer79]

Overall looks good to me, mostly reformating and reording the existing document, please let me know if there's anything specific that I should pay closer attention.

I will merge the PR if there's no further comment from @david-cortes .

After the merge, I will try to update the remaining files. Not so much is left now!