Load data to atomspace #20

Yidnekachew · 2018-07-09T12:53:32Z

Implemented based on #12

ngeiswei · 2018-07-10T05:00:40Z

moses/atomese/representation/atomese_representation.cc

+ * @param row_num row number
+ * @return
+ */
+Handle mk_row_number_cell(const int& row_num)


You don't need to pass int by const ref as int is as big as its pointer or smaller.

ngeiswei · 2018-07-10T05:10:33Z

moses/atomese/representation/atomese_representation.cc

+ * @param contin contin value of a cell
+ * @return
+ */
+Handle mk_real_cell(const contin_t& contin)


No need to pass contin by ref (it's the same size as a pointer).

ngeiswei · 2018-07-10T05:11:19Z

moses/atomese/representation/atomese_representation.cc

+ * @return
+ */
+Handle mk_real_cell_exec(const contin_t& contin,
+                         const int& row_num, const string& label)


Same remark about passing by ref (string though be passed by ref).

ngeiswei · 2018-07-10T05:11:39Z

moses/atomese/representation/atomese_representation.cc

+ * @param b builtin value
+ * @return
+ */
+Handle mk_boolean_cell(const builtin& b)


Same remark about builtin ref passing.

builtin is an enum. Shouldn't we pass it by ref?

ngeiswei · 2018-07-10T05:13:05Z

moses/atomese/representation/atomese_representation.cc

+	const type_node& type = oTable.get_type();
+	const string& label = oTable.get_label();
+	HandleSeq rows;
+	int row_num = 1;


I think it's better if it starts by 0 (contrary to what I've indicated on the issue). It makes the subsequent code slightly simpler and it's a more frequent start index value.

ngeiswei · 2018-07-10T05:23:47Z

moses/atomese/representation/atomese_representation.cc

+			                      mk_real_cell(boost::get<contin_t>(v)));
+			rows.push_back(h);
+		}
+	}


Alternatively you can put the conditional in the createLink and select either mk_real_cell or mk_boolean_cell depending on type. It slightly less efficient (though probably imperceptibly) but it reduces the number of lines of code.

Another even better option is to create a mk_cell visitor! that way you just call that mk_cell function on v and you're good. There are plenty of examples of visitors throughout MOSES' code (in table.h for instance). See also https://www.boost.org/doc/libs/1_67_0/doc/html/variant.html for its documentation.

It slightly less efficient (though probably imperceptibly) but it reduces the number of lines of code.

Will it be imperceptible as we're going to do the checking for every single cell in the data?

In fact, if we're changing it for only the output table, it wouldn't affect much. But, what if we're doing the same for other parts such as this line (https://github.com/opencog/as-moses/pull/20/files#diff-85f2b83d9bc9a6c48aeb22bb8535b034R325)?

ngeiswei · 2018-07-10T05:25:56Z

moses/atomese/representation/atomese_representation.cc

+ * 		 all features have similar type with the output feature.
+ * @return
+ */
+Handle mk_input_table(const ITable& iTable, const type_node& type)


No need to pass type_node by ref.

type_node is an enum too.

ngeiswei · 2018-07-10T05:28:11Z

moses/atomese/representation/atomese_representation.cc

+			HandleSeq cells;
+
+			cells.push_back(mk_boolean_cell(
+				boost::get<builtin>(table.otable[row_num - 1])));


If you start row_num from 0 you wouldn't need to subtract 1.

ngeiswei · 2018-07-10T05:36:54Z

@Yidnekachew it is OK if you make you next commits on top the existing one (i.e. not overwrite the git history) I mean the branch is clean enough at this point.

ngeiswei · 2018-07-10T10:41:29Z

@Yidnekachew said

Will it be imperceptible as we're going to do the checking for every single cell in the data?

Yes because the branch prediction of the CPU will choose the right function after the first few cycles.

ngeiswei · 2018-07-10T10:48:16Z

moses/atomese/representation/atomese_representation.cc

+			HandleSeq cells;
+
+			for (contin_t cell_value : m.get_seq<contin_t>())
+				cells.push_back(mk_real_cell(cell_value));


Hmm, you could simplify here as well by having the whole push_back loop wrapped in a visitor, but maybe it's just too much worry. I mean this code is already quite clean as it is.

ngeiswei · 2018-07-10T10:51:28Z

moses/atomese/representation/atomese_representation.cc

+				cells.push_back(mk_boolean_cell(cell_value));
+
+			rows.push_back(
+				similarity ? mk_numbered_row(cells, row_num + 1)


You can drop + 1 in row_num + 1 (in that and the other instances).

ngeiswei · 2018-07-10T10:59:39Z

moses/atomese/representation/atomese_representation.cc

+				for (builtin cell_value : m.get_seq<builtin>())
+					cells.push_back(
+						mk_boolean_cell_exec(cell_value, row_num + 1,
+						                     labels[cell_number++]));


Hmm, yes, I feel that if you wrap that code in a visitor you may simply a lot your code. Maybe like having a visitor for turning a multi_type_seq into a cells vector.

ngeiswei · 2018-07-12T04:16:35Z

moses/atomese/representation/atomese_representation.cc

+	HandleSeq operator()(const T& t) const
+	{}
+
+	int cell_number = 0;


It's a bit dangerous to have this here because if you reapply the visitor it will crash. I would suggest to move it in the operators.

ngeiswei · 2018-07-12T04:22:03Z

moses/atomese/representation/atomese_representation.cc

+
+	for (const multi_type_seq& m : table.itable)
+	{
+		HandleSeq cells = boost::apply_visitor(mk_cell_seq_visitor(),


BTW, it terms of performance, it might be better to create the visitor out of the loop. But frankly it's not even sure as both the compiler and system (CPU branch prediction) optimize that sort of things. Just keep that in mind for the day you want to speed up that function (although it's unlikely it'll be problematic).

ngeiswei · 2018-07-12T04:23:57Z

moses/atomese/representation/atomese_representation.cc

+ * @param table
+ * @return
+ */
+Handle mk_unfolded_boolean_table_eval(const Table& table)


ngeiswei · 2018-07-12T04:25:30Z

moses/atomese/representation/atomese_representation.cc

+	{
+		mk_exec_cell_seq_visitor cseqv;
+		cseqv.row_num = row_num;
+		cseqv.labels = labels;


I think C++11 allows you to write directly

mk_exec_cell_seq_visitor cseqv{row_num, labels};

ngeiswei · 2018-07-12T04:26:52Z

moses/atomese/representation/atomese_representation.cc

+	{}
+
+	int row_num;
+	string label;


Better be a const string&

ngeiswei · 2018-07-12T04:28:01Z

moses/atomese/representation/atomese_representation.cc

+	{}
+
+	int cell_number = 0;
+	HandleSeq cellSeq;


Better move this in the operators as well.

ngeiswei · 2018-07-12T04:28:20Z

moses/atomese/representation/atomese_representation.cc

+	int cell_number = 0;
+	HandleSeq cellSeq;
+	int row_num;
+	vector<string> labels;


Better be a const ref.

ngeiswei · 2018-07-13T06:19:30Z

moses/atomese/representation/load_table.cc

@@ -0,0 +1,474 @@
+/** atomese_representation.cc ---


Need name update. Your IDE likely offers a way to find and replace a string across multiple files, if not you can go old school with sed (though interactivity is usually better handled via an IDE).

ngeiswei · 2018-07-13T06:23:09Z

moses/atomese/representation/load_table.cc

+	mk_exec_cell_seq_visitor(const int row_num,
+	                         const vector<string>& labels)
+		: row_num(row_num), labels(labels)
+	{}


I think you don't need to define that constructor, C++11 initializer will work regardless.

I'm unable to do that.

My bad, they need to be public for it to work. You may either use struct instead of class or declare them public.

It's a struct now.

Cool, so now you should be able to remove mk_exec_cell_visitor and mk_exec_cell_seq_visitor constructors.

It was already struct and doesn't work if constructors removed.

Oh I see, C++11 aggregate initialization is very stringent, see https://en.cppreference.com/w/cpp/language/aggregate_initialization for more info.

It actually works with -std=c++17 but then this produces other problems with the cogutil tree class. And C++17 is too recent to force users to use a compiler compatible with it.

So you're gonna have to let it here, but you may add a comment that such a constructor isn't needed for C++17.

ngeiswei · 2018-07-13T06:26:51Z

tests/atomese/representation/atomese_reprUTest.cxxtest

+using namespace opencog;
+using namespace std;
+
+class atomese_reprUTest : public CxxTest::TestSuite {


Please update that unit test name (and the filename it's in) to follow the functions it is testing), load_tableUTest would do.

ngeiswei · 2018-07-13T06:27:33Z

moses/atomese/representation/load_table.h

@@ -0,0 +1,238 @@
+/** atomese_representation.h ---


ngeiswei · 2018-07-13T06:27:42Z

moses/atomese/representation/load_table.h

+ */
+
+#ifndef _OPENCOG_ATOMESE_REPRESENTATION_H
+#define _OPENCOG_ATOMESE_REPRESENTATION_H


ngeiswei · 2018-07-13T21:42:23Z

Last thing @Yidnekachew please fix the warnings (by having the default visitor operators return something, like Handle::UNDEFINED or {}).

ngeiswei · 2018-07-17T19:34:50Z

I've merged, it seems alright, including the git history.

Thanks for your patience and for putting up with my stringent requirements.

Yidnekachew · 2018-07-17T19:40:44Z

@ngeiswei It's an honor to get your comments!

Lbs test

Yidnekachew added 3 commits July 5, 2018 14:29

Add guile as a dependency

2378eba

implement different atomese loaders

c326e1d

implement unit tests for atomese loaders

fda1d45

ngeiswei reviewed Jul 10, 2018

View reviewed changes

create make_cell_visitor and remove unnecessary const refs

c31c48e

ngeiswei reviewed Jul 10, 2018

View reviewed changes

Yidnekachew added 2 commits July 11, 2018 11:49

create cell visitors

46fc49d

start row numbering from 0

2b3a0e1

ngeiswei reviewed Jul 12, 2018

View reviewed changes

ngeiswei reviewed Jul 13, 2018

View reviewed changes

rename atomese_representation to load_table

c77511b

fix warnings

bb64731

ngeiswei merged commit c038445 into opencog:master Jul 17, 2018

Yidnekachew deleted the load-data-to-atomspace branch July 17, 2018 19:40

ngeiswei added a commit to ngeiswei/asmoses that referenced this pull request Nov 1, 2018

Merge pull request opencog#20 from Bitseat/LBS_test

65c5ad8

Lbs test

Load data to atomspace #20

Load data to atomspace #20

Conversation

Yidnekachew commented Jul 9, 2018

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

Yidnekachew Jul 10, 2018 • 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

ngeiswei commented Jul 10, 2018

ngeiswei commented Jul 10, 2018 • 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

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

ngeiswei Jul 13, 2018 • 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

ngeiswei commented Jul 13, 2018

ngeiswei commented Jul 17, 2018

Yidnekachew commented Jul 17, 2018

Yidnekachew Jul 10, 2018 •

edited

Loading

ngeiswei commented Jul 10, 2018 •

edited

Loading

ngeiswei Jul 13, 2018 •

edited

Loading