Develop

[ivan-pavlov]

I still see an issue with std::err, I will fix that first. And I am currently skipping that test which fails every time. This's a strange thing because if I execute the same code outside of the test, everything works fine. I will learn more about how testthat works to solve this.

[eddelbuettel]

I am not sure what you are doing here but when I see something like

with 129 changed files I suspect something is not right. I think it is late where you are, so why don't you take a deep breath, get some sleep and look at this tomorrow with fresh eyes?

[ivan-pavlov]

Oh Yes, maybe I should really.

Maybe I misunderstood, but I thought you asked to start with PR from develop to master. Master is based on RVowpalWabbit so there should a lot of differences between this two versions.

[coatless]

@ivan-pavlov

• I think @eddelbuettel would like a bit more clarity on the title of the PRs
  • For example, the meaning of "Develop" is very generic. It would be better to label PRs as:
    • "Added Feature x"
    • "Changed ... feature to ... "
• RE: Large branch differences, the underlying code in master was RVowpalWabbit instead of rwv and, thus, the large amount of file modifications.

As it is late where you are, I would recommend picking this up tomorrow.

[eddelbuettel]

There is a distinct possibility that James and I had not been very clear.

There is a also a distinct possibility that you misunderstood us. Communication is hard. We all will try to do better.

Now, what most open source projects do is to

• start from a given point zero, call that master
• make a small targeted set of changes, maybe in a branch feature/more_cowbell
• and/or even another branch feature/better_bells
• and then the diff between a branch and master is
  • small
  • meaningful
  • accessible
  • ideally tested (new unit tests, no breakage of existing Travis)

None of that applies here when you change 129 files. I am in no position to judge this.

Now, getting from your rvwgsoc "fork" back to rvw may indeed be big. So for now, let us try to focus on something that is a) small, b) comprehensible -- ie from your branch back to your master.

Does that make sense? And do get some sleep.

[eddelbuettel]

And your last commit looks good! If I do this the cerr warning goes away:

diff --git a/src/rvw.cpp b/src/rvw.cpp
index 772e571..cbba711 100644
--- a/src/rvw.cpp
+++ b/src/rvw.cpp
@@ -14,7 +14,7 @@ void create_cache(std::string dir="", std::string data_file="", std::string cach

     // Redirect cerr
   std::stringstream new_buf;
-  auto old_buf = std::cerr.rdbuf(new_buf.rdbuf());
+  //auto old_buf = std::cerr.rdbuf(new_buf.rdbuf());

   std::string cache_init_str = "-d " + data_str + " -c --cache_file " + cache_str;
   vw* cache_model = VW::initialize(cache_init_str);
@@ -34,7 +34,7 @@ void create_cache(std::string dir="", std::string data_file="", std::string cach
   VW::end_parser(*cache_model);
   VW::finish(*cache_model);

-  std::cerr.rdbuf(old_buf);
+  //std::cerr.rdbuf(old_buf);

   // Rcpp::Rcout << "Cache file created: " << cache_str << std::endl;

@@ -54,7 +54,7 @@ void vwtrain(Rcpp::List vwmodel, std::string data_path="") {

   // Redirect cerr
   std::stringstream new_buf;
-  auto old_buf = std::cerr.rdbuf(new_buf.rdbuf());
+  //auto old_buf = std::cerr.rdbuf(new_buf.rdbuf());

   std::string train_init_str = Rcpp::as<std::string>(vwmodel["params_str"]);
   train_init_str += " -d " + data_str;
@@ -70,7 +70,7 @@ void vwtrain(Rcpp::List vwmodel, std::string data_path="") {
   VW::finish(*train_model);

   Rcpp::Rcout <<  new_buf.str() << std::endl;
-  std::cerr.rdbuf(old_buf);
+  //std::cerr.rdbuf(old_buf);
 }

 // [[Rcpp::export]]
@@ -94,7 +94,7 @@ Rcpp::NumericVector vwtest(Rcpp::List vwmodel, std::string data_path="", std::st

   // Redirect cerr
   std::stringstream new_buf;
-  auto old_buf = std::cerr.rdbuf(new_buf.rdbuf());
+  //auto old_buf = std::cerr.rdbuf(new_buf.rdbuf());

   std::string test_init_str = Rcpp::as<std::string>(vwmodel["params_str"]);
   test_init_str += " -t -d " + data_str + " -p " + probs_str + " -i " + model_str;
@@ -110,7 +110,7 @@ Rcpp::NumericVector vwtest(Rcpp::List vwmodel, std::string data_path="", std::st
   VW::finish(*predict_model);

   Rcpp::Rcout << new_buf.str() << std::endl;
-  std::cerr.rdbuf(old_buf);
+  //std::cerr.rdbuf(old_buf);

   Rcpp::NumericVector data_vec(num_of_examples);
   std::ifstream probs_stream (probs_str);

As I said, just a matter of commenting things out that are not needed.

As for the testthat error, maybe it is simply not able to write? Maybe ../my_tmp should be a real temporary file ?

[eddelbuettel]

Similar, the error in the example goes away with

edd@rob:~/git/rvwgsoc(develop)$ git diff -w R/functions.R
diff --git a/R/functions.R b/R/functions.R
index 58fef2a..4bfe616 100644
--- a/R/functions.R
+++ b/R/functions.R
@@ -79,7 +79,7 @@
 #'@return vwmodel list class
 #'@examples
 #'vwsetup(
-#'  dir = "../my_tmp/",
+#'  dir = tempdir(), #"../my_tmp/",
 #'  train_data = "X_train.vw",
 #'  test_data = "X_valid.vw",
 #'  model = "pk_mdl.vw",
edd@rob:~/git/rvwgsoc(develop)$ 

I just commented out the test for vwsetup().