pwep commented 5 years ago

Hi Dev,

From the compiler error you sent, there is a redefinition of the erf(double) function, which clashes with an existing standard library version.
Commenting out the function in RmsFuncs.cpp should remove the clash. I was not getting a compile error on my machines, but CRAN/solaris will be configured slightly different.
The function does not appear to be called from any other file (R or C++), and within RmsFunc.cpp the erfcpp(double) function appears to be used instead.
The erfcpp(double) function is not exposed to R, but could be if required.

codecov[bot] commented 5 years ago

Codecov Report

Merging #3 into master will increase coverage by <.01%. The diff coverage is 0%.

@@            Coverage Diff            @@
##           master      #3      +/-   ##
=========================================
+ Coverage    0.08%   0.08%   +<.01%     
=========================================
  Files          56      56              
  Lines        7325    7323       -2     
=========================================
  Hits            6       6              
+ Misses       7319    7317       -2

Impacted Files	Coverage Δ
R/PlotRsmOperatingCharacteristics.R	`0% <0%> (ø)`	:arrow_up:
src/RsmFuncs.cpp	`1.51% <0%> (+0.04%)`	:arrow_up:

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 0cc50c3...6cc7d85. Read the comment docs.

dpc10ster commented 5 years ago

Hi Dev,

From the compiler error you sent, there is a redefinition of the erf(double) function, which clashes with an existing standard library version.

Commenting out the function in RmsFuncs.cpp should remove the clash. I was not getting a compile error on my machines, but CRAN/solaris will be configured slightly different.

The function does not appear to be called from any other file (R or C++), and within RmsFunc.cpp the erfcpp(double) function appears to be used instead.

The erfcpp(double) function is not exposed to R, but could be if required.

Hi Peter,

erfcpp() function is called from PlotRsmOperatingCharacteristics.R, so it needs to be exposed to R.
I tried merging your code with mine but got nowhere - it makes sense now, because you hadn't pushed it!

Thanks, I need your help and it opens up possibilities.

Dev

pwep commented 5 years ago

Ah! So you have R functions defined in PlotRsmOperatingCharacteristics.R which share the same name as functions in RmsFuncs.cpp. When the functionis called, the R version is run, and not the C++ code.

Currently a call to erfcpp() runs the function as defined at the end of the PlotRsmOperatingCharacteristics.R file. We need to:

[ ] Keep but rename the R implementation of erf (currently called erfcpp as found in PlotRsmOperatingCharacteristics.R
[ ] Rename the erf function, in as defined in RmsFuncs.cpp, to erfcpp, and make available to R

pwep commented 5 years ago

Running Rcpp::compileAttributes() will force a regeneration of the the RcppExports.R and RcppExports.cpp files, but I have included the updated files in commit 6cc7d85.

dpc10ster commented 5 years ago

Ah! So you have R functions defined in PlotRsmOperatingCharacteristics.R which share the same name as functions in RmsFuncs.cpp. When the functionis called, the R version is run, and not the C++ code.

Currently a call to erfcpp() runs the function as defined at the end of the PlotRsmOperatingCharacteristics.R file. We need to:

[ ] Keep but rename the R implementation of erf (currently called erfcpp as found in PlotRsmOperatingCharacteristics.R

[ ] Make available to R the erfcpp() function, as defined in RmsFuncs.cpp

You got it! I believe I performed a 'quick-fix' remedy to get past some previous compiler errors, and since speed is not important in this function, I coded erfcpp() in R. I think your second option is better; alternatively, if the error function exists in all cpp libraries, maybe we should use that function instead. Your call on this.
Another issue: when I run R CMC CHK I get a note:

❯ checking installed package size ... NOTE installed size is 5.1Mb sub-directories of 1Mb or more: doc 2.5Mb

0 errors ✔ | 0 warnings ✔ | 1 note ✖

Any ideas? I want to add more vignettes, as these help the user, but the file size is a limitation. Could vignettes be in a separate package? Would that solve the problem?

Thanks again, Peter.

dpc10ster commented 5 years ago

Hi Peter, Let me know when you are done and when I can merge your changes. Please tell me how to do it. Can I just click on the Merge pull request button?

Dev

dpc10ster commented 5 years ago

Peter,

My Skype address is dev.chakraborty. Could we chat sometime?

Dev

pwep commented 5 years ago

I'll dust of my Skype account. I am pwephillips on Skype. It might be a good idea for you to create new branch in git, called development. We can merge pull requests into the development branch. When a new release is ready, the development branch can be merged into the master branch (and tagged as a release).

dpc10ster commented 5 years ago

OK, I will create the development branch next; We can Skype later;

Dev

On Jun 19, 2019, at 3:06 PM, Peter Phillips notifications@github.com wrote:

I'll dust of my Skype account. I am pwephillips on Skype. It might be a good idea for you to create new branch in git, called development. We can merge pull requests into the development branch. When a new release is ready, the development branch can be merged into the master branch (and tagged as a release).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/dpc10ster/RJafroc/pull/3?email_source=notifications&email_token=AH4NJRFBDSTZX5YZ7VGDGJDP3J7UNA5CNFSM4HZEXLV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYC3FZQ#issuecomment-503689958, or mute the thread https://github.com/notifications/unsubscribe-auth/AH4NJRBB44RNZL6SC3XLUATP3J7UNANCNFSM4HZEXLVQ.

dpc10ster commented 5 years ago

Done

On Jun 19, 2019, at 3:06 PM, Peter Phillips notifications@github.com wrote:

I'll dust of my Skype account. I am pwephillips on Skype. It might be a good idea for you to create new branch in git, called development. We can merge pull requests into the development branch. When a new release is ready, the development branch can be merged into the master branch (and tagged as a release).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/dpc10ster/RJafroc/pull/3?email_source=notifications&email_token=AH4NJRFBDSTZX5YZ7VGDGJDP3J7UNA5CNFSM4HZEXLV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYC3FZQ#issuecomment-503689958, or mute the thread https://github.com/notifications/unsubscribe-auth/AH4NJRBB44RNZL6SC3XLUATP3J7UNANCNFSM4HZEXLVQ.

dpc10ster / RJafroc

Erf fix #3

Codecov Report