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

Modules vignette review #982

Merged

Conversation

@riccardoporreca
Copy link
Contributor

riccardoporreca commented Jul 25, 2019

Review of the Rcpp-modules vignette, with major focus on the usage in packages, as a follow-up of #976 (comment)

A (not so short) summary of the changes:

  • Replaced sparse references to packages across a few existing examples in the Rpp Modules section with a pointer to the specific section at beginning.

  • Introduced a new sub-section about usaging sourceCpp() as an alternative to cxxfuntion() + getDynLib().

  • Package section reviewed with focus on recommended loading via loadModule()

    • A more practical example has been included
    • The section about the deprecated loading was left as-is
    • Namespace exports are also covered.
  • Improvements and harmomization of existing examples in section Rcpp Modules

    • General review to make it easier for a user to follow the examples.
    • All examples now assume fx (clearly stated) and explicitly call Module(), to make them close to self-contained.
    • Include World class module in S4 dispatch example for completeness.
    • Make sure all examples (with R code) are runnable, and optionally enable running them
      • Non-included chunks creating the assumed relevant fx object based on the C++ code in previous named chunk(s).
      • The document has gained parameters eval and results in the YAML header to control evaluating the examples on demand (e.g. Knit with parameters...).
      • This is pretty non-intrusive and can be easily removed.
    • Running the examples revealed compilation errors with the final std::vector example, which I fixed with a workaround to be discussed.
  • Fixed a few tyops + batch cosmetic alignment:

    • \proglang all the way.
    • \textsl{Rcpp modules} everywhere but in their own section.
    • Aligned and fixed usage of C++ elements in inline text.

I left a few explicit comments marked as CHECK-PR to have some pointers for the PR discussion.

@riccardoporreca

This comment has been minimized.

Copy link
Contributor Author

riccardoporreca commented Jul 25, 2019

More on the issue with the std::vector example.

The following is a minimal example

#include <Rcpp.h>
typedef std::vector<double> vec;
// Free function wrappers to work around errors
// error: call of overloaded
// ‘method(const char [7], <unresolved overloaded function type>)’ is ambiguous
void vec_resize (vec* obj, int n) {
    obj->resize(n);
}
// error: no matching function for call to
// ‘Rcpp::class_<std::vector<double> >::method(const char [10], <unresolved overloaded function type>)’
void vec_push_back (vec* obj, double value) {
    obj->push_back(value);
}
RCPP_MODULE(mod_vec) {
    using namespace Rcpp;
    class_<vec>("vec")
    .constructor<int>()
    // exposing member functions
    .method("resize", &vec::resize)
    // .method("resize", &vec_resize)
    .method("push_back", &vec::push_back)
    // .method("push_back", &vec_push_back)
    ;
}

With compilation errors (Ubuntu 18.04, compiler details: g++ -std=gnu++11). This are probably errors introduced with C++11 where both push_back and resize are overloaded compared to C++11

void push_back (const value_type& val);
void push_back (value_type&& val);
void resize (size_type n);
void resize (size_type n, const value_type& val);

This does not seem to be supported by .const_method and .nonconst_method in Rcpp

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jul 25, 2019

Lovely stuff. I will make some time to review. Trying to get some other things out the door first so may take a few days. But first off ... big thanks for this!

Copy link
Contributor

coatless left a comment

This is great. I made a few remarks for when @eddelbuettel looks over it.

vignettes/Rcpp-modules.Rmd Outdated Show resolved Hide resolved
vignettes/Rcpp-modules.Rmd Outdated Show resolved Hide resolved
vignettes/Rcpp-modules.Rmd Show resolved Hide resolved
vignettes/Rcpp-modules.Rmd Outdated Show resolved Hide resolved
vignettes/Rcpp-modules.Rmd Show resolved Hide resolved
vignettes/Rcpp-modules.Rmd Outdated Show resolved Hide resolved
into the package namespace:

```{r}
loadModule("yada", TRUE)

This comment has been minimized.

Copy link
@coatless

coatless Jul 26, 2019

Contributor

Perhaps switch this to a namespace function call?

Suggested change
loadModule("yada", TRUE)
Rcpp::loadModule("yada", TRUE)

This comment has been minimized.

Copy link
@riccardoporreca

riccardoporreca Aug 5, 2019

Author Contributor

This is surely a valid point, although this approach does not seem to be used across other Rcpp vignettes.

vignettes/Rcpp-modules.Rmd Outdated Show resolved Hide resolved
```{r, echo=FALSE,eval=TRUE}
options( prompt = " ", continue = " " )
```

```{r, eval=FALSE}
```{r}

This comment has been minimized.

Copy link
@coatless

coatless Jul 26, 2019

Contributor

I feel like there was also a need to add:

import(methods)

This comment has been minimized.

Copy link
@riccardoporreca

riccardoporreca Aug 5, 2019

Author Contributor

I double-checked using the package produced by Rcpp::Rcpp.package.skeleton(), removed the DESCRIPTION and NAMESPACE imports of methods and things seem to still work.

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Aug 6, 2019

Member

Thanks for checking that. I will merge this PR but that whole aspect may be worth looking into in another ticket. This changed over the years, and maybe describing just one simple (working) setup is best.

vignettes/Rcpp-modules.Rmd Outdated Show resolved Hide resolved
@riccardoporreca

This comment has been minimized.

Copy link
Contributor Author

riccardoporreca commented Jul 26, 2019

@eddelbuettel, @coatless, I was happy to provide my small contribution after being a happy user for quite some time.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jul 28, 2019

This is really good work, and I like it a lot. I didn't get to running it though.

I think we should work out what we want the sections / questions you labeled as 'CHECK-PR' to say before we merge.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Aug 3, 2019

I truly apologize for taking so long. Life gets in the way at times.

My verdict is mostly two thumbs way up. I would just ask for one more pass, removing all the CHECK-PR as detailed below.

I may still have overlooked some issues, but my quick notes are below.

PS We also moved master ahead in the meantime so maybe you want to rebase or else we can simplify and squash-merge for simplicity (once you made the small editing changes suggested below).

Comments:

  • CHECK-PR on parameters: I'd say remove, in-line with what all other vignettes do. Not a
    bad idea though. (We do conditional evaluation of chunks in the drat-data
    paper in the R Journal, so the idea is fine in principle.)

  • So let's restore evaluation as before

  • All the typo fixes are excellent. Thank you so much!

  • Ditto with all the editing fixes. Very attentive.

  • CHECK-PR on resize: Likely name clash and good idea to work around. So we
    should keep this and replace 'CHECK-PR' with something like 'FixForC++11'

  • CHECK-PR on mustStart=TRUE: Don't know

  • CHECK-PR on sourceCpp: Excellent. Absolutely keep. Nicely done.

  • CHECK-PR on chunk needed: That looks like a left-over style option from
    JSS or something. Not needed I'd say.

* As a preliminary step before reviewing the specific section about packges.
* General refinement to make it easier for a user to follow and run the examples.
* Now all inline examples assume fxx (clearly stated) and explicitly call Module(), to make the examples closer to self-contained.
* Also fixed minor typos.
* World was not already included in the examples above.
* Inline examples with R code now have a non-included chunk creating the assumed relevant fx object based on the C++ code in previous named chunk(s).
* The document has gained a parameter `eval` in the YAML header, that can be set to TRUE to optionally run the examples.
* This revealed compilation errors with the std::vector example, fixed with a workaround, to be discussed upon PR.
* Focus on recommended usage via loadModule with example * General review, also covering exports.
* New section about using the convenient sourceCpp.
* \proglang all the way.
* \textsl{Rcpp modules} everywhere but in their own section.
* Consistent usage of inline C++ elements.
* Make evaluation even less intrusive.
* Removed parameters for evaluating chunks, restored evaluation as before, keeping named Rcpp chunks.
* Finalized comment about resize and push_back workarounds for C++11.
* Referred to Rcpp attributes vignette for sourceCpp.
* Removed mustStart=TRUE (undocumented), the example works without.
* Removed left-over style option chunks not needed.
@riccardoporreca riccardoporreca force-pushed the riccardoporreca:modules-vignette-pkg-review branch from 7d3fb4f to 9832314 Aug 5, 2019
@riccardoporreca

This comment has been minimized.

Copy link
Contributor Author

riccardoporreca commented Aug 5, 2019

@eddelbuettel, @coatless, thanks for your feedback.

Back to connected life after a long weekend, I have addressed the CHECK-PR points in 9832314 as suggested above, with details in the commit comment.

I have rebased the branch, although squash-merging might actually be a good idea.

Copy link
Contributor

coatless left a comment

LGTM!

@eddelbuettel eddelbuettel merged commit 199b6e4 into RcppCore:master Aug 6, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
eddelbuettel added a commit that referenced this pull request Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.