Design improve #37

Banana1530 · 2019-07-03T15:10:02Z

Two main focuses in this PR:

Add three member functions for the class MoMA.
A comprehensive test suite for cpp_multirank_BIC_grid_search, which will be the core of multivariate methods.

- separate class RcppFourDList declaration and implementation - update R extractor to deal with 5-D list

- include MoMA::Omega_u/v - remove MoMA::tol and MoMA::iter - add flags `is_initialzied` and `is_solved` - add helper functions `initialize_uv`, `set_X`, `check_convergence` and `evaluate_loss` (NOT IMPLEMENTED YET) - add "level-1" function `multi_rank`, `grid_search` and `criterion_search`, and put them in `moma_level1.cpp` - modify the almighty function `grid_BIC_mix`

- improve `check_convergence` - remove `tol` and `iter` (working precision and iters)

- three "level-1" functions are exposed in `moma_expose.cpp` - they share a common R interface in `moma_expose.R`

michaelweylandt

First set of comments. Continuing review.

michaelweylandt · 2019-07-03T15:19:07Z

R/moma_5Dlist_extractor.R

+    n_lambda_u <- dim(x)[2]
+    n_alpha_v <- dim(x)[3]
+    n_lambda_v <- dim(x)[4]
+    n_rank <- dim(x)[5]


I haven't found how to set it in styler. Could you point it out?

From the README, it looks like setting strict = FALSE won't mess with existing alignment but I don't see how to force my type of alignment.

Don't worry about it - we'll just commit to what styler uses. I was commenting out of habit.

michaelweylandt · 2019-07-03T15:20:20Z

R/moma_5Dlist_extractor.R

@@ -0,0 +1,37 @@
+get_5Dlist_elem <- function(x, alpha_u_i, lambda_u_i, alpha_v_i, lambda_v_i, rank_i = 1) {


Longer term, we should think about making an accessor which takes alpha_u, lambda_u, etc (values not indices) and does the extraction. If we don't have exactly the right value in the saved list, we should (by default) interpolate with an option for an exact solve.

(I do something similar for the coef function in my ExclusiveLasso package.)

I opened an issue to track this.

#40 (comment)

michaelweylandt · 2019-07-03T15:21:54Z

src/moma.cpp

 };

 int MoMA::deflate(double d)
 {
-    MoMALogger::warning("Deflating.");
+    MoMALogger::debug("Deflating:\n")


Maybe a message? This doesn't quite seem debug-level.

I'd also include which rank is being used here.

Oh - and just an FYI: I'm working on a new short paper on alternate deflation schemes. Will share when it's ready.

I think we should keep it as debug message since we also output << X << "u^T = " << u.t() << "v^T = " << v.t() << "d = u^TXv = " << d; (next line). This could be lengthy.

That's great! Can't wait to see it.

michaelweylandt · 2019-07-03T15:23:13Z

src/moma.cpp

@@ -114,107 +119,55 @@ void MoMA::solve()
        u = solver_u.solve(X * v, u);
        v = solver_v.solve(X.t() * u, v);

-        tol = norm(oldu - u) / norm(oldu) + norm(oldv - v) / norm(oldv);
-        MoMALogger::debug("Outer loop No.") << iter << "--" << tol;
+        double scale_u = norm(oldu) == 0.0 ? 1 : norm(oldu);


What norm function is this? Should it be arma::norm?

Yes. Fixed.

src/moma.cpp

michaelweylandt · 2019-07-03T15:27:07Z

src/moma.cpp

+    // NOTE: We must keep the alpha's and lambda's up-to-date
+    // in both MoMA and solve_u/v
+    if (std::abs(alpha_u - newalpha_u) > EPS || std::abs(alpha_v - newalpha_v) > EPS ||
+        std::abs(lambda_v - newlambda_v) > EPS || std::abs(lambda_u - newlambda_u) > EPS)


Alignment - can we line up the || in the first and second line?

A clearer way to write this might be std::max(......) > EPS instead of checking each condition

I searched for it for 10 minutes and did not find such option. clang-format won't allow me to align them by hand.

True, but let's keep it this way. I changed the EPS here to MOMA_FLOATPOINT_EPS .

Ok - it's harmless.

michaelweylandt · 2019-07-03T15:28:42Z

src/moma_expose.cpp

@@ -82,7 +64,7 @@ Rcpp::List cpp_sfpca_grid(
    double EPS_inner,
    long MAX_ITER_inner,
    std::string solver,
-    int k = 1)
+    int rank = 1)  // `rank` is not used


Why have this argument if not used? At least, maybe we should throw an error if rank != 1 to catch user mistakes.

The reason why I want it here is to make the three member functions have the same argument lists. See below:

https://github.com/Banana1530/MoMA-1/blob/afea2740f489994d6213b618ca5005c6da61e208/R/moma_expose.R#L171

Checking added. See commit "check users don't specify rank".

michaelweylandt · 2019-07-03T15:29:10Z

src/moma_expose.cpp

@@ -173,7 +115,7 @@ Rcpp::List cpp_sfpca_nestedBIC(
    double EPS_inner,
    long MAX_ITER_inner,
    std::string solver,
-    int k = 1)
+    int rank = 1)  // rank not used


Same comment as above - why have this?

michaelweylandt · 2019-07-03T15:30:16Z

src/moma_expose.cpp

+
+// This function solves a squence of lambda's and alpha's
+// [[Rcpp::export]]
+Rcpp::List cpp_multirank_BIC_grid_search(


This name seems a bit over-loaded since it doesn't always do BIC or GS. Maybe just cpp_multirank_svd or something?

michaelweylandt · 2019-07-03T15:32:40Z

src/moma_fivedlist.cpp

+    flattened_list.attr("class") = "MoMA_5D_list";
+};
+
+int RcppFiveDList::insert(Rcpp::List object,


This might be more clearly written with the ()= paradigm than an insert function.

You mean an () operator for the C++ class RcppFiveDList ?

I think so.

If we add RcppFiveDList::operator(), wont' that let us write

list(1,2,3,4,5) = Rcpp::List::create(....)

in the code that uses this class?

I opened an issue to track this.

#41 (comment)

michaelweylandt

Test code looks solid on first pass.

michaelweylandt · 2019-07-03T15:38:38Z

src/moma_level1.cpp

+    // grid_au = alpha_u, bic_au_grid = [-1].
+    // If alpha_u is selected via nested BIC search,
+    // then grid_au = [-1], bic_au_grid = alpha_u
+    const arma::vec &grid_lu = set_greedy_grid(lambda_u, !selection_criterion_lambda_u);


Why is this a const ref object instead of just a const object? I'm a bit confused by the semantics. (TBH: I didn't know you could just declare an object as a const ref outside of a function call context.)

const ref because I want grid_lu to be either a reference of the vector lambda_u, when the parameter lambda_u is determined by grid search, so the vector lambda_u should constitute a dimension of the so-called "greedy grid" - see the doc linked below; or the MOMA_EMPTY_GRID_OF_LENGTH1 (defined in moma_base.h), when lambda_u is determined BIC.

We can do this as long as the ~~functions~~ function returns a const ref.

Well it doesn't have to be a ref. const will do just fine.

Interesting - that's a weird corner of C++ I hadn't played with before, but makes sense.

michaelweylandt · 2019-07-03T15:38:49Z

src/moma_level1.cpp

+
+    RcppFiveDList five_d_list(n_alpha_u, n_lambda_u, n_alpha_v, n_lambda_v, rank);
+
+    arma::mat original_X =


Yes. See commit "const arma::mat original_X".

michaelweylandt · 2019-07-03T15:39:44Z

src/moma_level1.cpp

+            {
+                for (int m = 0; m < n_lambda_v; m++)
+                {
+                    arma::vec bic_au_grid = set_bic_grid(alpha_u, selection_criterion_alpha_u, i);


I'm still confused as to what these set_*_grid functions do. Can you clarify?

https://docs.google.com/document/d/1XKnQpF4SDaWy8e4ynHGoEiJOEy8sj07-0r5wXjnWd7s/edit?usp=sharing

See here.

I think I get what you are trying to do, but the names of the C++ functions are still a bit confusing.

Maybe construct_grid_for_search and construct_grid_no_search or something like that?

Changed. See commit "set_*_gric -> construct_*_search".

michaelweylandt · 2019-07-09T03:22:20Z

Anything else you think we should do for this PR or is it good to merge?

Banana1530 · 2019-07-09T07:54:08Z

Let's merge this PR. But one thing of concern is how to deal with deflation for CCA in current design.

Banana1530 added 8 commits July 3, 2019 20:38

5-D list

60665a0

- separate class RcppFourDList declaration and implementation - update R extractor to deal with 5-D list

Imporve class _PR_solver

bb135ca

- improve `check_convergence` - remove `tol` and `iter` (working precision and iters)

Better comments and debug message for BIC_searcher

5165555

Expose three "level-1" functions to R

d8b46c5

- three "level-1" functions are exposed in `moma_expose.cpp` - they share a common R interface in `moma_expose.R`

Update wrappers' names

2574511

Test suites for cpp_multirank_BIC_grid_search

d5b6b30

Remove old tests

afea274

Banana1530 requested a review from michaelweylandt July 3, 2019 15:10

michaelweylandt reviewed Jul 3, 2019

View reviewed changes

Banana1530 mentioned this pull request Jul 5, 2019

Accessor with interpolation #40

Open

Banana1530 added 5 commits July 5, 2019 21:22

norm -> arma::norm

fad5234

naming and floating point comparison in MoMA::evaluate_loss

c7c1949

Error when calling MoMA::evaluate_loss

d76d64f

check users don't specify rank

f4ecb03

const arma::mat original_X

3582225

Banana1530 mentioned this pull request Jul 9, 2019

Add RcppFiveDList::operator() #41

Open

set_*_gric -> construct_*_search

14c37b6

Banana1530 force-pushed the designimprove branch from cf20213 to 14c37b6 Compare July 9, 2019 07:48

Merge branch 'master' into designimprove

79e6050

michaelweylandt approved these changes Jul 9, 2019

View reviewed changes

michaelweylandt merged commit cc9be2e into DataSlingers:master Jul 9, 2019

Banana1530 deleted the designimprove branch July 26, 2019 06:48

Banana1530 mentioned this pull request Aug 21, 2019

GSoC (Google Summer of Code) 2019 Report #55

Closed

		@@ -0,0 +1,37 @@
		get_5Dlist_elem <- function(x, alpha_u_i, lambda_u_i, alpha_v_i, lambda_v_i, rank_i = 1) {


		RcppFiveDList five_d_list(n_alpha_u, n_lambda_u, n_alpha_v, n_lambda_v, rank);

		arma::mat original_X =

Design improve #37

Design improve #37

Conversation

Banana1530 commented Jul 3, 2019

michaelweylandt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelweylandt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Banana1530 Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

Banana1530 Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelweylandt commented Jul 9, 2019

Banana1530 commented Jul 9, 2019

Banana1530 Jul 3, 2019 •

edited

Loading

Banana1530 Jul 5, 2019 •

edited

Loading