Add Quic_SVD method in CF #3404

AdarshSantoria · 2023-02-09T04:35:58Z

As mentioned in #3398, Quic_SVD method is added in CF and some standard changes in 'methods/quic_svd` are performed so that it can be used in cf.

rcurtin

Thanks for taking the time to add this! I agree with the changes to the QUIC_SVD class itself. Do you think you could also add a test for this in cf_test.cpp? It should be pretty simple, since there are already templated tests that are in use for the other decomposition policies.

rcurtin · 2023-02-09T19:17:48Z

src/mlpack/methods/cf/decomposition_policies/quic_svd_method.hpp

+    arma::mat tempMat = w;
+    w = h;
+    h = tempMat;
+  }


Can you fix the style here please? I'm guessing that there are some tab characters. mlpack code should be all spaces (no tabs). See also https://github.com/mlpack/mlpack/wiki/DesignGuidelines#style-guidelines 👍

src/mlpack/methods/quic_svd/quic_svd.hpp

src/mlpack/methods/cf/decomposition_policies/quic_svd_method.hpp

Co-authored-by: Ryan Curtin <ryan@ratml.org>

AdarshSantoria · 2023-02-09T19:43:10Z

Thanks for taking the time to add this! I agree with the changes to the QUIC_SVD class itself. Do you think you could also add a test for this in cf_test.cpp? It should be pretty simple, since there are already templated tests that are in use for the other decomposition policies.

Yes I will add some tests. Thanks for all suggestions.

conradsnicta · 2023-02-10T11:44:59Z

suggest to also add a corresponding item to HISTORY.md

rcurtin

Thanks for adding this! The implementation looks mostly right, just a few minor comments throughout. 👍

rcurtin · 2023-02-13T16:08:05Z

src/mlpack/methods/cf/decomposition_policies/quic_svd_method.hpp

+ * cf.GetRecommendations(10, recommendations);
+ * @endcode
+ */
+class QuicSVDPolicy


Since the original class is called QUIC_SVD, I think we should call this QUIC_SVDPolicy to be consistent. (I know that is harder to type...)

rcurtin · 2023-02-13T16:08:41Z

src/mlpack/methods/quic_svd/quic_svd.hpp

@@ -42,21 +42,20 @@ namespace mlpack {
 * const double epsilon = 0.01; // Relative error limit of data in subspace.
 * const double delta = 0.1 // Lower error bound for Monte Carlo estimate.
 *
+ * // Make a QuicSVD object.
+ * QuicSVD qSVD();


Shouldn't this be QUIC_SVD?

rcurtin · 2023-02-13T16:10:44Z

src/mlpack/tests/cf_test.cpp

+TEST_CASE("EmptyConstructorTrainQSVDTest", "[CFTest]")
+{
+  EmptyConstructorTrain<QuicSVDPolicy>();
+}


Just an idea either for this or a future cleanup... I learned about TEMPLATE_TEST_CASE() from Catch recently, and so we could combine all of these tests into something like this:

TEMPLATE_TEST_CASE("EmptyConstructorTrainTest", "[CFTest]", RandomizedSVDPolicy, RegSVDPolicy, ..., QuicSVDPolicy) { EmptyConstructorTrain<TestType>(); }

Sure I will submit a fresh pr for this as it will cause some conflicts in #3413. Thanks for the idea.

You don't need to do a fresh PR... you can just handle the merge conflicts in #3413, it shouldn't be too hard. But your call, either way is fine. 👍

rcurtin · 2023-02-13T16:14:07Z

src/mlpack/methods/cf/decomposition_policies/quic_svd_method.hpp

+
+    // Do singular value decomposition using the quic SVD algorithm.
+    QUIC_SVD quicsvd;
+    quicsvd.Apply(data, w, h, sigma);


QUIC_SVD gives the approximate SVD data = u * diagmat(sigma) * v.t(); but your code here ignores sigma. I think that's incorrect and sigma should be incorporated into either w or h, so that we can reconstruct as data = w * h.t().

AdarshSantoria · 2023-02-13T21:10:42Z

src/mlpack/methods/quic_svd/quic_svd_impl.hpp

@@ -64,6 +82,9 @@ inline void QUIC_SVD::ExtractSVD(arma::mat& u,
  arma::vec sigmaBar;
  arma::svd(uBar, sigmaBar, vBar, projectedMatSquared);

+  // vBar was tarnsposed of vBar
+  vBar = arma::trans(vBar);


It is mentioned here that we get transposed of vBar after svd link mentioned in 'quic_svd.hpp`.

I'm extremely surprised this change here doesn't break the tests in quic_svd_test.cpp. I am not sure if the implementation returns exactly what the algorithm says, but we should not make this change: it fundamentally changes how QUIC_SVD works and will break user expectations. As the implementation stands (see the test), the original matrix should be reconstructed as u * sigma * v.t(). Let's not change that---instead if you need a transpose for CFType, do it in QUIC_SVDPolicy.

rcurtin

One small implementation comment, and then just a bunch of style fixes. Everything is looking good to me. 👍

src/mlpack/tests/cf_test.cpp

rcurtin · 2023-02-14T16:17:42Z

src/mlpack/methods/quic_svd/quic_svd_impl.hpp

@@ -64,6 +82,9 @@ inline void QUIC_SVD::ExtractSVD(arma::mat& u,
  arma::vec sigmaBar;
  arma::svd(uBar, sigmaBar, vBar, projectedMatSquared);

+  // vBar was tarnsposed of vBar
+  vBar = arma::trans(vBar);


I'm extremely surprised this change here doesn't break the tests in quic_svd_test.cpp. I am not sure if the implementation returns exactly what the algorithm says, but we should not make this change: it fundamentally changes how QUIC_SVD works and will break user expectations. As the implementation stands (see the test), the original matrix should be reconstructed as u * sigma * v.t(). Let's not change that---instead if you need a transpose for CFType, do it in QUIC_SVDPolicy.

src/mlpack/methods/quic_svd/quic_svd_impl.hpp

Co-authored-by: Ryan Curtin <ryan@ratml.org>

rcurtin

Awesome---thank you for handling all the comments over the various rounds of review. This is a nice contribution! 👍

mlpack-bot

Second approval provided automatically after 24 hours. 👍

rcurtin · 2023-02-17T03:29:07Z

Thanks @AdarshSantoria! 👍

AdarshSantoria added 2 commits February 9, 2023 09:22

add quic_svd for cf

b99e7cf

fixing comments

a57b561

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Feb 9, 2023

rcurtin reviewed Feb 9, 2023

View reviewed changes

AdarshSantoria and others added 3 commits February 10, 2023 00:53

Update src/mlpack/methods/cf/decomposition_policies/quic_svd_method.hpp

0efb5ac

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/methods/quic_svd/quic_svd.hpp

c223e64

Co-authored-by: Ryan Curtin <ryan@ratml.org>

fix style

4d8f000

AdarshSantoria and others added 5 commits February 12, 2023 12:55

Added preprocessing part and tests

6c51f3d

Add comment

3315aff

Update HISTORY.md

b32355d

fix typo

2b7bb73

Revert Changes

6309e47

AdarshSantoria mentioned this pull request Feb 12, 2023

Update HISTORY.md #3414

Merged

rcurtin reviewed Feb 13, 2023

View reviewed changes

fix quicsvd bug and add template tests

d11cb3f

AdarshSantoria commented Feb 13, 2023

View reviewed changes

rcurtin reviewed Feb 14, 2023

View reviewed changes

AdarshSantoria and others added 9 commits February 14, 2023 21:53

Update src/mlpack/methods/quic_svd/quic_svd_impl.hpp

6066623

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

70539c8

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

bcacf4e

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

32eaf10

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

3f9367c

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

84e4ec7

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

fd58849

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

30f5763

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

ddd491a

Co-authored-by: Ryan Curtin <ryan@ratml.org>

AdarshSantoria and others added 6 commits February 14, 2023 21:58

Revert change

b760dd7

Update src/mlpack/tests/cf_test.cpp

81033aa

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

ad5e1c4

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

35a82ec

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

089863f

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/tests/cf_test.cpp

b8190d1

Co-authored-by: Ryan Curtin <ryan@ratml.org>

rcurtin approved these changes Feb 15, 2023

View reviewed changes

mlpack-bot bot approved these changes Feb 16, 2023

View reviewed changes

mlpack-bot bot removed the s: needs review label Feb 16, 2023

rcurtin merged commit d6657f1 into mlpack:master Feb 17, 2023

rcurtin mentioned this pull request Apr 27, 2023

Release version 4.1.0 #3476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Quic_SVD method in CF #3404

Add Quic_SVD method in CF #3404

AdarshSantoria commented Feb 9, 2023

rcurtin left a comment

rcurtin Feb 9, 2023

AdarshSantoria commented Feb 9, 2023

conradsnicta commented Feb 10, 2023

rcurtin left a comment

rcurtin Feb 13, 2023

rcurtin Feb 13, 2023

rcurtin Feb 13, 2023

AdarshSantoria Feb 13, 2023 •

edited

rcurtin Feb 13, 2023

rcurtin Feb 13, 2023

AdarshSantoria Feb 13, 2023

rcurtin Feb 14, 2023

rcurtin left a comment

rcurtin Feb 14, 2023

rcurtin left a comment

mlpack-bot bot left a comment

rcurtin commented Feb 17, 2023

Add Quic_SVD method in CF #3404

Add Quic_SVD method in CF #3404

Conversation

AdarshSantoria commented Feb 9, 2023

rcurtin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdarshSantoria commented Feb 9, 2023

conradsnicta commented Feb 10, 2023

rcurtin 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

AdarshSantoria Feb 13, 2023 • edited

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

rcurtin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcurtin left a comment

Choose a reason for hiding this comment

mlpack-bot bot left a comment

Choose a reason for hiding this comment

rcurtin commented Feb 17, 2023

AdarshSantoria Feb 13, 2023 •

edited