search

d- / MTS

Multivariate Time Series Package for R
4 stars 5 forks source link

Rewrite the VARMA likelihood and refactor the code a lot #4

Closed jonlachmann closed 2 years ago

jonlachmann commented 2 years ago

Hi! I have rewritten the VARMA likelihood in C++, and it seems to be roughly 6 times faster now. I have also used Rd2roxygen and styler to format the code according to the standards.

It would be great if it was possible to merge these changes to eventually get them to CRAN.

Best regards Jon Lachmann

d- commented 2 years ago

Thanks for the work Jon. Could we separate fixes and other substantial changes from reformatting changes so that we can review and test the PR. As is 119 files were modified - it's simply too big a PR to act.

Here is some context that might be helpful. https://betterprogramming.pub/how-to-make-a-perfect-pull-request-3578fb4c112

d- commented 2 years ago

Thanks for the work Jon. Could we separate fixes and other substantial changes from reformatting changes so that we can review and test the PR. As is 119 files were modified - it's simply too big a PR to act.

Here is some context that might be helpful. https://betterprogramming.pub/how-to-make-a-perfect-pull-request-3578fb4c112

Maybe we should revert to this commit for now? https://github.com/d-/MTS/commit/c206aceb3e6d49f2d0bbcc0535d9e4018b6bc927

jonlachmann commented 2 years ago

Hi, yes, you are correct. I was in a hurry to get everything done. I will rework it to make one PR for the new likelihood in C++, and then I can make a separate one for the reformatting after we have gone through the first one. The reason I did all the restyling is that I use the tools for Rcpp to generate the exports, and they didnt work without some reformatting of the code.. and then everything sped up from there =)

jonlachmann commented 2 years ago

I have cleaned up the PR and made one just with the new varma likelihood here https://github.com/d-/MTS/pull/5. Feel free to close this PR.