Skip to content

Fix add cor gen 2 #178

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

Merged
merged 21 commits into from
Nov 17, 2022
Merged

Fix add cor gen 2 #178

merged 21 commits into from
Nov 17, 2022

Conversation

kgoldfeld
Copy link
Owner

This update involves some major changes to addCorGen and moves some R code to Rcpp to improve performance substantially.

@kgoldfeld
Copy link
Owner Author

@assignUser I made some pretty big changes to addCorGen that speed things up (moved some functions to Rcpp) and make it more flexible. I am quite sure I didn't break anything in the process. There weren't a lot of tests in this part of the code, and I didn't get to it here. I'll do that soon, but need to get this out sooner.

There was one problem with a number of servers (windows oldrel and macos oldrel) that couldn't find the package pbv which I used in the new Rcpp function for the ep method. For some reason that I don't understand, this was a problem with one of my examples. I removed that example, and everything is good. But that didn't really solve the problem - I'll note is an issue and try to fix at some point before we move to CRAN.

If this all sounds good, I'll merge for now.

@assignUser
Copy link
Collaborator

@kgoldfeld I won't have time before the weekend to look this over in detail. Feel free to merge if this is blocking you or users. I can still add comments on a closed PR :) I will also fix the benchmarking action this weekend so we can see the improvements! 🚀

@kgoldfeld
Copy link
Owner Author

That sounds good. We can always fix/clean any more issues later. Thanks.

@kgoldfeld kgoldfeld merged commit f940cae into main Nov 17, 2022
@assignUser
Copy link
Collaborator

@kgoldfeld I have fixed lorenzwalthert/touchstone#118 so the benchmark workflow should be fixed now 👍

@kgoldfeld
Copy link
Owner Author

kgoldfeld commented Nov 21, 2022

@assignUser That is great to hear. I was wondering how to deal with the issue that at least two of the servers that we are running R CMD CHECK on are giving an error indicating they are not finding the pbv package (or last a specific function within that package). Is it possible that the server doesn't have the latest version of that package loaded? If not, do you have any idea how we could make that happen? I am afraid if I develop any tests for the updated genCorGen and addCorGen functions, they will generate errors in the R CMD CHECK. Do you know who maintains those servers, or are they not even actual servers, but something virtual?

@assignUser
Copy link
Collaborator

I have check out the logs,the latest CRAN version of pbv is installed but for some reason on oldrel and devel a specific Rcpp function of the package can not be found... I have little experience with Rcpp so I am not sure what the issue could be but there is a more recent version of the package on github. I look into it on the weekend.

@kgoldfeld
Copy link
Owner Author

kgoldfeld commented Nov 21, 2022

@assignUser I can only think of two possibilities. One is that the package isn't updated enough. Two is the issues of "LinkTo" and "Imports" in the Description file. If a package is used in Rcpp, it must be linked to, but not imported if it is not used in any of the R programs. I had it in "Imports" originally but was getting warnings since it is not used anywhere else, so I took it out.

@assignUser
Copy link
Collaborator

@kgoldfeld this seems to happen only with oldrel (so R 4.1.3) you could test if that issue also happens locally with that r version and devtools::check() (or test() should even be enough?)

@kgoldfeld
Copy link
Owner Author

Thanks - I'll give it a try.

@kgoldfeld kgoldfeld deleted the fix-addCorGen-2 branch April 3, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants