[Lib][ML] Replace std::vector usage with Vector #116

TatianaJin · 2016-11-07T09:21:53Z

Fix issue #115

Change parameters to Vector<T, false>
Modify FeatureLabel and unify data and gradient vector type

kygx-legend · 2016-11-07T09:25:37Z

lib/CMakeLists.txt

@@ -27,6 +27,4 @@ set(lib-objs $<TARGET_OBJECTS:aggregator-objs>)
 # Visible to parent directory
 set(lib-objs ${lib-objs} PARENT_SCOPE)

-add_subdirectory(ml)


No errors if removing this?

There is no cpp under ml dir to be compiled as the previous vector_linalg.cpp is substituted by Vector class.

All right, a little scary.

kygx-legend

And could some thoughts in issue #117 be applied here?

kygx-legend · 2016-11-08T05:09:44Z

examples/linear_regression.cpp

-    data_loader.load_info(husky::Context::get_param("test"), test_set);
-    int num_features = data_loader.get_num_feature();
+    auto& format = husky::lib::ml::kTSVFormat;
+    // int num_features = std::stoi(husky::Context::get_param("num_features"));


Please remove comments like this.

Comment still there?

Kelvin-Ng · 2016-11-10T11:43:03Z

I have made changes on DataPoint according to issue #117 in pull request #125. Please update your code accordingly.

TatianaJin · 2016-11-11T09:59:17Z

@Kelvin-Ng Okay.

TatianaJin · 2016-11-21T12:20:30Z

Hi @kygx-legend, this PR is updated. Please check.

kygx-legend · 2016-11-21T13:45:52Z

Thanks for the contribution! I'll check this tomorrow carefully.

kygx-legend · 2016-11-22T05:46:46Z

lib/ml/data_loader.hpp


 namespace husky {
 namespace lib {
 namespace ml {
-typedef std::vector<double> vec_double;
-typedef std::vector<std::pair<int, double>> vec_sp;

 // indicate format
 const int kLIBSVMFormat = 0;
 const int kTSVFormat = 1;


Can we use enum instead?

Kelvin-Ng · 2016-11-07T11:23:44Z

lib/ml/feature_label.hpp

-template <typename FT, typename LT>
-class FeatureLabelBase {
+template <typename FeatureT, typename LabelT, bool is_sparse>
+class FeatureLabel : public DataPoint<FeatureT, LabelT, is_sparse> {


I think the naming here is a bit confusing. I think it is better to have class FeatureLabelSomeSuffix : public FeatureLabel, class DataPointSomeSuffix : public DataPoint etc. Actually I want to change the class name from DataPoint to LabeledPoint, so I think class LabeledPointSomeSuffix : public LabeledPoint

Kelvin-Ng · 2016-11-21T13:48:59Z

examples/linear_regression.cpp

@@ -47,35 +47,36 @@
 #include "lib/ml/linear_regression.hpp"
 #include "lib/ml/scaler.hpp"
 #include "lib/ml/sgd.hpp"
+#define is_sparse false


Why?!
Even if this is true, use const bool is_sparse = false instead.

Kelvin-Ng · 2016-11-21T14:07:16Z

examples/linear_regression.cpp

 using husky::lib::ml::ParameterBucket;
+using SparseFeatureLabel = husky::lib::ml::FeatureLabel<double, double, is_sparse>;


When is_sparse being a constant set to false (although I think it should not be), how can this be sparse?

Kelvin-Ng · 2016-11-21T14:10:22Z

examples/linear_regression.cpp

-    data_loader.load_info(husky::Context::get_param("train"), train_set);
-    data_loader.load_info(husky::Context::get_param("test"), test_set);
-    int num_features = data_loader.get_num_feature();
+    auto& format = husky::lib::ml::kTSVFormat;


Better to let the user choose the file format in config file?

Kelvin-Ng · 2016-11-21T14:15:10Z

lib/ml/feature_label.hpp

-class FeatureLabelBase {
+// extends LabeledPoint, using Vector to represent features
+template <typename FeatureT, typename LabelT, bool is_sparse>
+class FeatureLabel : public LabeledPoint<Vector<FeatureT, is_sparse>, LabelT> {


The name looks confusing...

What is your suggestion on the naming convention?

LabeledPointWithId? But this is a bit long...

Or LabeledPointObj? The suffix is long so it is unavoidable that the subclass name becomes long.

I think this is not a good name because anything is an object...

Actually, I don't prefer abbreviate. It's more confusing. Long is okay if it's meaningful enough.

What does LabeledPointImpl mean?

The suffix Impl is not so suitable because the original class LabeledPoint is already a complete class but not an interface.

How about LabeledPointHObj, where HObj is the short form of Husky object?

I'm fine with this.

Then I would further propose that we set a naming convension that:
All 'Husky object' version of a general class Foo should be named FooHObj.

What do you think about that?

ddmbr · 2016-11-23T03:53:52Z

Seems this Linear Regression only works for sparse data? It would be better if it can be more general purpose, and support more data formats (by making it configurable). Otherwise you have to rename some parts or write enough comments to avoid confusion because these things are in lib/.

Kelvin-Ng · 2016-11-23T03:56:32Z

@ddmbr The truth is the opposite. The Linear Regression works only for dense data. The so-called SparseFeatureLabel is actually dense. That's so confusing.

TatianaJin · 2016-11-23T04:38:13Z

@ddmbr Yes I am working on it.
@Kelvin-Ng Actually it can be both sparse and dense, and I wrote SparseFeatureLabel when I started with is_sparse=true and later I change it to false to test if dense data is ok.

Kelvin-Ng · 2016-11-23T05:16:13Z

OK. But anyway please use a less confusing name.

TatianaJin · 2016-11-23T08:54:41Z

@ddmbr Now the sparsity and format of data are configurable.

Fix issue husky-team#115 1. Change parameters to Vector<T, false> 2. Modify FeatureLabel and unify data and gradient vector type 3. LinearRegression and LogisticRegression object can now be properly moved / copied.

zzxx-husky · 2016-11-28T07:15:00Z

Not an easy PR.

kygx-legend reviewed Nov 7, 2016

View reviewed changes

TatianaJin force-pushed the dev branch from 684a0a6 to 6b6c7dd Compare November 7, 2016 09:58

zzxx-husky mentioned this pull request Nov 7, 2016

[IO][Input] Add InputformatFactory to handle create/release #111

Merged

kygx-legend mentioned this pull request Nov 8, 2016

Naming and other miscellaneous things about husky::lib::{Vector,DataPoint} #117

Closed

kygx-legend requested changes Nov 8, 2016

View reviewed changes

TatianaJin force-pushed the dev branch from 6b6c7dd to 7766d44 Compare November 8, 2016 15:21

This was referenced Nov 11, 2016

[Lib][ML] DataLoader cannot be used twice #126

Closed

[Lib][ML] Move template specialization of constructor of DataLoader to data_loader.cpp #129

Closed

TatianaJin force-pushed the dev branch 2 times, most recently from e6b817a to 998f9a5 Compare November 21, 2016 12:07

kygx-legend added this to the v0.1.0 milestone Nov 21, 2016

kygx-legend reviewed Nov 22, 2016

View reviewed changes

kygx-legend approved these changes Nov 22, 2016

View reviewed changes

Kelvin-Ng reviewed Nov 22, 2016

View reviewed changes

TatianaJin force-pushed the dev branch 2 times, most recently from da62adb to c111d98 Compare November 23, 2016 05:54

[Lib][ML] Replace std::vector usage with Vector

0650299

Fix issue husky-team#115 1. Change parameters to Vector<T, false> 2. Modify FeatureLabel and unify data and gradient vector type 3. LinearRegression and LogisticRegression object can now be properly moved / copied.

TatianaJin force-pushed the dev branch from c111d98 to 0650299 Compare November 24, 2016 08:48

kygx-legend assigned zzxx-husky Nov 28, 2016

zzxx-husky merged commit a1624e9 into husky-team:master Nov 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lib][ML] Replace std::vector usage with Vector #116

[Lib][ML] Replace std::vector usage with Vector #116

TatianaJin commented Nov 7, 2016

kygx-legend Nov 7, 2016

TatianaJin Nov 7, 2016

kygx-legend Nov 7, 2016

kygx-legend left a comment

kygx-legend Nov 8, 2016

kygx-legend Nov 14, 2016

Kelvin-Ng commented Nov 10, 2016

TatianaJin commented Nov 11, 2016

TatianaJin commented Nov 21, 2016

kygx-legend commented Nov 21, 2016

kygx-legend Nov 22, 2016

Kelvin-Ng Nov 7, 2016

Kelvin-Ng Nov 21, 2016

Kelvin-Ng Nov 21, 2016

Kelvin-Ng Nov 21, 2016

Kelvin-Ng Nov 21, 2016

TatianaJin Nov 23, 2016

Kelvin-Ng Nov 23, 2016

TatianaJin Nov 23, 2016

Kelvin-Ng Nov 23, 2016

kygx-legend Nov 24, 2016

TatianaJin Nov 24, 2016

Kelvin-Ng Nov 24, 2016

kygx-legend Nov 24, 2016

Kelvin-Ng Nov 24, 2016

ddmbr commented Nov 23, 2016

Kelvin-Ng commented Nov 23, 2016

TatianaJin commented Nov 23, 2016

Kelvin-Ng commented Nov 23, 2016

TatianaJin commented Nov 23, 2016

zzxx-husky commented Nov 28, 2016

		using husky::lib::ml::ParameterBucket;
		using SparseFeatureLabel = husky::lib::ml::FeatureLabel<double, double, is_sparse>;

[Lib][ML] Replace std::vector usage with Vector #116

[Lib][ML] Replace std::vector usage with Vector #116

Conversation

TatianaJin commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kygx-legend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kelvin-Ng commented Nov 10, 2016

TatianaJin commented Nov 11, 2016

TatianaJin commented Nov 21, 2016

kygx-legend commented Nov 21, 2016

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

ddmbr commented Nov 23, 2016

Kelvin-Ng commented Nov 23, 2016

TatianaJin commented Nov 23, 2016

Kelvin-Ng commented Nov 23, 2016

TatianaJin commented Nov 23, 2016

zzxx-husky commented Nov 28, 2016