fix sign() to handle NaN

[conradsnicta]

This commit fixes the sign() function to properly handle NaN values.
When NaN is given, it now correctly returns NaN instead of incorrectly returning zero.
Related bug report: https://gitlab.com/conradsnicta/armadillo-code/-/issues/169

[eddelbuettel]

I am sorry but why do you suggest all of a sudden to patch your code in our package? I mean 🤷‍♂️ sure why not but why not rely on what we have for ten years and have your release signify your release?

[eddelbuettel]

And of course, don't get me wrong a bug is a bug and a bug needs fixing but ... around here people look first for the Rcpp (Sugar) vectorised functions, and that one works:

> library(Rcpp)
> cppFunction("arma::vec arma_sign(arma::vec d) { return arma::sign(d); }", depends="RcppArmadillo")
> sapply(c(-1,0,1,NA,NaN), arma_sign)
[1] -1  0  1  0  0
> 
> cppFunction("NumericVector rcpp_sign(NumericVector d) { NumericVector x(sign(d)); return x;}")
> sapply(c(-1,0,1,NA,NaN), rcpp_sign)
[1] -1  0  1 NA NA
>  

So I think we can just wait this out to the next upgrade.

[conradsnicta]

1. The rcpp::sign() function is not arma::sign(). The rcpp::sign() function doesn't work with armadillo vectors and matrices.
2. As discussed via email late last year, we agreed to reduce the churn in RcppArmadillo releases (ie. keep RcppArmadillo at 0.10.1.x for a long while). I stated that I will send you bug fixes directly as pull requests via GitHub. This is one of the fixes.

[eddelbuettel]

Dear Conrad, please consider:

Re 1: Yes, and R users almost always call Rcpp:: functions first. Which is why this PR, while technically helpful, may not matter.

Re 2: We have posted some markdown files in various places and stated our position repeatedly: a strong preference for issue tickets and discussion leading to some consensus before unsolicited PRs. Now, granted, this PR is fairly local and focussed but .... this is simply not the right way to go about things.

Especially as every other library I cover as a port or integration to R via Rcpp works via their own well managed git repsository. Yours was always a little, ahem, "special" with your insistence on tarballs with overwritten file timestamps etc. Your code, your repo, your rules, so at some point years ago I gave up pointing this out. This here, however, is our repo so I would appreciate if you could play by its rules. And issue ticket pointed to a commit of yours (over at GitLab) may suffice and make life a lot easier for you too.

[eddelbuettel]

Also as discussed last time, it is preferred that PRs come with a corresponding ChangeLog entry in the standard format identifying the author, file, and purpose.

[codecov-io]

Codecov Report

Merging #321 (193c4fa) into master (399c204) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #321   +/-   ##
=======================================
  Coverage   34.29%   34.29%           
=======================================
  Files          65       65           
  Lines        2356     2356           
=======================================
  Hits          808      808           
  Misses       1548     1548           

---

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 399c204...193c4fa. Read the comment docs.

[eddelbuettel]

Incorporporated this by hand in db84fc2 so PR no redundant --> closing