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

Use R's RNG in default example included in skeleton package? #69

Closed
rstub opened this issue Mar 29, 2019 · 12 comments
Closed

Use R's RNG in default example included in skeleton package? #69

rstub opened this issue Mar 29, 2019 · 12 comments

Comments

@rstub
Copy link
Contributor

@rstub rstub commented Mar 29, 2019

return x + (y-x) * Scalar(std::rand()) / Scalar(RAND_MAX);

Shouldn't R's RNG be used here?

(Reference: https://stat.ethz.ch/pipermail/r-package-devel/2019q1/003687.html)

@eddelbuettel

This comment has been minimized.

Copy link
Member

@eddelbuettel eddelbuettel commented Mar 29, 2019

I am a little disappointed by this Ralf. You surely know that a non-included header is of no impact in a shared library, no?

And for what it is worth I do not consider myself to be in the business of policing / editing / patching tens of thousands of that many files. If you think the world needs a 'BHplus' please suggest it to CRAN.

@rstub

This comment has been minimized.

Copy link
Contributor Author

@rstub rstub commented Mar 29, 2019

Maybe I am more dense than normal, but I do not understand why you are refering to a "non-included header" file. The above quoted file is included if one includes RcppEigen.h. As it turns out, the default skeleton package using RcppEigen triggers the NOTE from R CMD check that rand() should not be used:

$  R -e "RcppEigen::RcppEigen.package.skeleton('EigenRand')"
[...]
$ R CMD build EigenRand
[...]
$ R CMD check EigenRand_1.0.tar.gz
[...]
* checking compiled code ... NOTE
File ‘EigenRand/libs/EigenRand.so’:
  Found ‘rand’, possibly from ‘rand’ (C)
    Object: ‘rcppeigen_hello_world.o’

Compiled code should not call entry points which might terminate R nor
write to stdout/stderr instead of to the console, nor use Fortran I/O
nor system RNGs.

See ‘Writing portable packages’ in the ‘Writing R Extensions’ manual.
* checking examples ... OK
* checking PDF version of manual ... OK
* DONE

Status: 1 NOTE
See
  ‘/tmp/EigenRand.Rcheck/00check.log’
for details.

$ nm -A -g -C EigenRand.Rcheck/00_pkg_src/EigenRand/src/*o | grep rand
EigenRand.Rcheck/00_pkg_src/EigenRand/src/EigenRand.so:                 U rand@@GLIBC_2.2.5
EigenRand.Rcheck/00_pkg_src/EigenRand/src/rcppeigen_hello_world.o:                 U rand

Probably due to this line:

Eigen::MatrixXd m2 = Eigen::MatrixXd::Random(3, 3);

One might argue that this case is pathologic. I am not sure, though, how the OP of that thread on r-pkg-devel managed to trigger that NOTE.

@eddelbuettel eddelbuettel reopened this Mar 29, 2019
@eddelbuettel

This comment has been minimized.

Copy link
Member

@eddelbuettel eddelbuettel commented Mar 29, 2019

Hm. Did anything change there? Lemme reopen then...

@eddelbuettel

This comment has been minimized.

Copy link
Member

@eddelbuettel eddelbuettel commented Mar 29, 2019

Oh, I see now what you mean. That is a stupid example -- will change to calling R's RNG.

@eddelbuettel eddelbuettel changed the title Use R's RNG? Use R's RNG in default example included in skeleton package? Mar 29, 2019
@eddelbuettel

This comment has been minimized.

Copy link
Member

@eddelbuettel eddelbuettel commented Mar 29, 2019

From the top of your head, any other good initializers you'd recomment from here ? Maybe LinSpaced ?

@eddelbuettel

This comment has been minimized.

Copy link
Member

@eddelbuettel eddelbuettel commented Mar 29, 2019

Draft:

Eigen::MatrixXd rcppeigen_hello_world() {
    Eigen::MatrixXd m1 = Eigen::MatrixXd::Identity(3, 3);
    //Eigen::MatrixXd m2 = Eigen::MatrixXd::Random(3, 3);
    Eigen::MatrixXd m2 = Eigen::MatrixXd::Zero(3, 3);
    for (auto i=0; i<m2.rows(); i++)
        for (auto j=0; j<m2.cols(); j++)
            m2(i,j) = R::rnorm(0, 1);

    return m1 + 3 * (m1 + m2);
}
@rstub

This comment has been minimized.

Copy link
Contributor Author

@rstub rstub commented Mar 29, 2019

That removes the NOTE for the default skeleton, but the fundamental problem remains. One can trigger Eigen into using std::rand() even in less obvious ways than calling a method called Random. For example, the following function also triggers the rand() NOTE:

Eigen::MatrixXd rcppeigen_hello_world() {
    Eigen::MatrixXd m1 = Eigen::MatrixXd::Identity(3, 3);
    //Eigen::MatrixXd m2 = Eigen::MatrixXd::Random(3, 3);
    Eigen::MatrixXd m2 = Eigen::MatrixXd::Zero(3, 3);
    for (auto i=0; i<m2.rows(); i++)
        for (auto j=0; j<m2.cols(); j++)
            m2(i,j) = R::rnorm(0, 1);

    Eigen::RealQZ<Eigen::MatrixXd> qz(3);
    qz.compute(m1,m2);
    
    return m1 + 3 * (m1 + m2);
}

since Eigen::RealQZ uses internal::random() (which in turn uses std::rand()) in some corner cases:

// extremely exceptional shift
x = internal::random<Scalar>(-1.0,1.0);
y = internal::random<Scalar>(-1.0,1.0);
z = internal::random<Scalar>(-1.0,1.0);

@eddelbuettel

This comment has been minimized.

Copy link
Member

@eddelbuettel eddelbuettel commented Mar 29, 2019

I am sorry. I do not understand what you're after here.

I believe we are both aware that Writing R Extensions and the R CMD check tests dislike the C library RNG. Which is why we are rewriting it in ways to NOT trigger it.

As opposed to enumerating all possible ways to trigger it. So why would RealQZ() help?

As I said at the outset, I do not consider myself here in the role of an editor or reviewer for Eigen code. If they include code that may trigger R warnings I officially do not care. I am here to support the basic use cases not involving rand.

@rstub

This comment has been minimized.

Copy link
Contributor Author

@rstub rstub commented Mar 29, 2019

I meanwhile think that the OP of that r-pkg-devel thread used the Random method. In that case it might be sufficient to document some restrictions:

Don't use Random() or RealQZ if you want to submit your package to CRAN.

@eddelbuettel

This comment has been minimized.

Copy link
Member

@eddelbuettel eddelbuettel commented Mar 29, 2019

Yes. Writing R Extension does that. R CMD check checks for it. But I also do not do house visits to patch their system's C library.

@rstub

This comment has been minimized.

Copy link
Contributor Author

@rstub rstub commented Mar 29, 2019

As I said at the outset, I do not consider myself here in the role of an editor or reviewer for Eigen code. If they include code that may trigger R warnings I officially do not care. I am here to support the basic use cases not involving rand.

Ok, fair enough. In that case the proposed change to rcppeigen_hello_world() is sufficient.

@eddelbuettel

This comment has been minimized.

Copy link
Member

@eddelbuettel eddelbuettel commented Mar 29, 2019

Good to have agreement. Shipping examples that trigger warnings and hence do not provide best practives is something I don't like so thanks for pointing me here. That had long been overlooked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.