search

esmucler / odpc

This package provides functions for computing One-Sided Dynamic Principal Components, a novel multivariate time series dimension reduction technique proposed in Peña, Smucler and Yohai (2019) (https://doi.org/10.1080/01621459.2018.1520117).
7 stars 4 forks source link

Minimal changes to use modified Fields behaviour in RcppArmadillo #4

Closed eddelbuettel closed 2 years ago

eddelbuettel commented 2 years ago

Please apply these two one-line changes to src/Makevars.in and src/Makevars.win

diff --git a/src/Makevars.in b/src/Makevars.in
index 7fd474c..cc4140a 100644
--- a/src/Makevars.in
+++ b/src/Makevars.in
@@ -1 +1,2 @@
-PKG_LIBS = $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS) @OPENMP_CFLAGS@
\ No newline at end of file
+PKG_CPPFLAGS = -DRCPP_ARMADILLO_FIX_Field
+PKG_LIBS = $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS) @OPENMP_CFLAGS@
diff --git a/src/Makevars.win b/src/Makevars.win
index 22ebc63..d441b87 100644
--- a/src/Makevars.win
+++ b/src/Makevars.win
@@ -1 +1,2 @@
+PKG_CPPFLAGS = -DRCPP_ARMADILLO_FIX_Field
 PKG_LIBS = $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS)

They each just add the line PKG_CPPFLAGS = -DRCPP_ARMADILLO_FIX_Field to ensure each compilation defines this declaration (and doing so is easier/quicker than adding it to each source file).

However, when you do, your behaviour in the package changes and examples and tests fail. Can you look into adapting the code?

If you can't or won't because the old (long-published but arguably 'buggy') behaviour by RcppArmadillo was useful for your case we can simply do the inverse and define the declaration of 'yes please use old RcppArmadillo behaviour' as describe in issue over in its repo . You are probably in a better position to judge how this should evolve.

Please let me know if I can help otherwise, I simply haven't had time to dig into internals of your package yet.

esmucler commented 2 years ago

Dirk, thanks for taking the time to go over this. I tried it out and indeed the tests fail, and I cannot figure out what goes wrong at the moment. I am pretty sure that the old "buggy" behaviour made my code behave as I intended, so for now I am opting to put the alternative declaration in place. I tried this out with RcppArmadillo 0.10.8.2.0 and everything seems to work out nicely; see here.

I guess if I submit this new version to CRAN this whole thing will be resolved for odpc, right?

Best, Ezequiel

PS: Also, thanks a lot for developing Rcpp and RcppArmadillo, they are fantastic.

eddelbuettel commented 2 years ago

Yes -- If you want to leave things 'as is' we need to add the 'opt-into' as you have done in the updated #3 and it only needs to be at CRAN before May when I plan to 'flip the switch'.