Created CUAIFile class (Issue #1913) to save and load file in UAI file format. #2019

abinashpanda · 2014-03-17T15:28:46Z

No description provided.

abinashpanda · 2014-03-17T15:29:58Z

CUAIFile implemented according to Issue #1913

hushell · 2014-03-17T18:54:08Z

src/shogun/io/UAIFile.cpp

+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * Written (W) 2014 Abinash Panda (gsomix)


I don't think you want to name after gsomix :)

hushell · 2014-03-17T19:30:22Z

@abinashpanda Good job! There are several issues, you may try to resolve. Make sure that function tables in UAI format are actually energy tables in our case. Please add unit-tests for the new class.

hushell · 2014-03-18T01:59:55Z

Also, please git reset --soft to make only one commit for this PR.

hushell · 2014-03-18T04:31:28Z

src/shogun/io/UAIFile.cpp

+#include <shogun/io/UAIFile.h>
+
+#include <shogun/lib/SGVector.h>
+#include <shogun/base/DynArray.h>


Please use shogun/lib/DynamicArray.h

hushell · 2014-03-18T04:43:09Z

parse() is great! But I think you missed how to construct a FactorGraph from the UAI file. Please think about how to do that. Something like SGVector::load(CFile*).

vigsterkr · 2014-03-18T07:14:32Z

src/shogun/io/UAIFile.cpp

+
+void CUAIFile::parse()
+{
+


why new line?

vigsterkr · 2014-03-20T09:33:22Z

any progress on this PR?

abinashpanda · 2014-03-20T09:34:26Z

@vigsterkr I would be completing it by today. I was busy with my GSoC applications and class test.

abinashpanda · 2014-03-21T07:08:28Z

@hushell @vigsterkr I have corrupted this PR with many commits and I have to still refactor it. So should I close this one and send a new PR?

vigsterkr · 2014-03-21T07:09:18Z

@abinashpanda corrupted how?

vigsterkr · 2014-03-21T07:10:59Z

src/shogun/io/UAIFile.cpp

+    m_tokenizer=NULL;
+    m_line_tokenizer=NULL;
+    m_parser=NULL;
+    m_line_reader=NULL;


SG_ADD is still missing... please check other shogun class implementation...

abinashpanda · 2014-03-21T07:17:13Z

@vigsterkr by corrupted means, I have messed up the implementation. Regarding SG_ADD: I am trying to implement this class as CSVFile has been implemented. I didn't find any SG_ADD. I you could suggest me some other class.

hushell · 2014-03-21T07:17:58Z

@abinashpanda Please don't launch another PR, since we have already discussed a lot on this one. Try to git reset --soft. This way people knows better what's going on here.

tklein23 · 2014-03-28T23:38:51Z

src/shogun/io/UAIFile.h

+    virtual void get_vector(uint16_t*& vector, int32_t& len);
+    virtual void get_vector(int64_t*& vector, int32_t& len);
+    virtual void get_vector(uint64_t*& vector, int32_t& len);
+    //@}


Can we try to use function templates here? IMO it whould really reduce the amount of duplicated code.

@tklein23 see CFile.h

tklein23 · 2014-03-28T23:41:25Z

Code itself looks good to me. I'm happy to see unit-tests for that.

But I'm really confused about 1000 lines for reading one file format. (@abinashpanda - I know it's not completely your fault, but it's a good time to discuss this.)

The header file contains about 450 lines, the implementation 350. Seems to be 90% boiler-plate-code. Is this really necessary or did we just massively duplicate code? Any suggestions how to improve it?

Any objections against using function templates to reduce the size of the header file? Shouldn't be a big deal - let me know if you need any assistance (i.e. documentation/examples) for this.

abinashpanda · 2014-03-29T06:11:24Z

@tklein23 By the function template implementation for get_vector, what I understood is template <class T> get_vector(T&* vector, int32_t& len) (correct me if I am wrong), but I think there are few problems associated with it:

First the vectors access functions as well as the matrix access functions are inherited from CFile, where they are not implemented using function templates, so I don't think they can be implemented here using function templates.
Secondly, the vector and matrix access functions cannot be implemented using function templates in CFile also as they must be virtual and I don't think C++ allows creation of virtual template function.
Any suggestions?

tklein23 · 2014-03-29T22:03:20Z

I'm convinced that it requires bigger changes. I'd say it's beyond the scope of this issue.

@hushell - as you created the issue, finishing this issue is up to you. Please check if we're done here.

hushell · 2014-03-30T07:46:01Z

@tklein23 I think since most of file loaders in Shogun are implemented in this way, we can keep it as this temporally. My intuition was not to make so much effort to create a UAI file loader.

@abinashpanda The code for UAI format part is good. Please fix the Travis issues, try to avoid using add_vector() to register members. I mean you don't need to stick on SGVector. Once you make Travis happy, we could merge this PR for you.

However, it seems you only finished 50% of issue #1913, we would like to use these code to build factor graphs. Well, since this PR has lived for a long time, let's finish it soon and start a new one for the rest of the task.

hushell · 2014-03-30T07:52:16Z

src/shogun/io/UAIFile.cpp

+
+    /** Can only be enable after this issue is https://github.com/shogun-toolbox/shogun/issues/1972
+     * resolved
+     * SG_ADD(m_factors_table, "m_factors_table", "table of factors", MS_NOT_AVAILABLE);


Interesting, in this way, it passed the clone tests. Use "factors_table" not "m_factors_table".

On Sunday 30 March 2014 00:52:18 Shell Hu wrote:

+}
+
+CUAIFile::~CUAIFile()
+{

SG_UNREF(m_tokenizer);

SG_UNREF(m_line_tokenizer);

SG_UNREF(m_parser);

SG_UNREF(m_line_reader);
+}

+void CUAIFile::init()
+{
+

/** Can only be enable after this issue is
Adding support for SGVector to Parameter::add_vector #1972 + * resolved

\* SG_ADD(m_factors_table, "m_factors_table", "table of factors",
MS_NOT_AVAILABLE);

Interesting, in this way, it passed the clone tests. Use "factors_table" not
"m_factors_table".

No surprise: No registration takes place, so it's simply serializing an
object without members.

If we're only missing support for SGVector, can't we just enable all
other members and keep the affected ones disabled until #1972 is solved?

hushell · 2014-03-30T08:00:38Z

It seems Travis issues have been solved, I am not sure we still have problems related to #1972.
@abinashpanda Before merging the PR, please valgrind to make sure no memory leak in your unit-tests.

vigsterkr · 2014-04-01T09:32:59Z

src/shogun/io/UAIFile.h

+namespace shogun
+{
+
+/** @brief Class UAIFILE used to read data from comma-separated values (CSV)


this class description is not true at all..

tklein23 · 2014-04-11T22:54:42Z

@abinashpanda - welcome back. I hope you are fully recovered.

What's the status of this PR?

Last Travis built failed because SerializationXML.UAIFile is broken. Do you know what happened here?

abinashpanda · 2014-04-12T05:28:44Z

@tklein23 - I am fully recovered now.
Regarding the PR, as per @karlnapf the SerializationXML.UAIFile breaking down might have to do with the generic type of the class or its members. It is set with the method
template<class T> void set_generic();
or unset with
void unset_generic();.
I tried to unset it using unset_generic() method, but still facing the same error. Any suggestions ?

hushell · 2014-04-12T06:22:47Z

@abinashpanda let's wrap this up. After getting over the serialization issue, you could choose to leave the 2nd half of the entrance task aside, i.e., no touching with factor graph stuffs. It will be nice to get familiar with SSVM solvers in Shogun in pre-GSoC period.

karlnapf · 2014-04-12T12:44:47Z

I suggest you copy the unit test template and fill in your class that fails by hand, have this as a separate Shogun program and then debug to see whats wrong. Let me know if that helps, sometimes this stuff is tricky

abinashpanda · 2014-04-13T05:13:38Z

@karlnapf Thanks for your suggestion. Finally I have modified the code and travis build passed. @tklein23 @hushell If you could review the code. Thanks in advance :)

hushell · 2014-04-13T08:04:22Z

src/shogun/io/UAIFile.cpp

+    SG_UNREF(m_parser);
+    SG_UNREF(m_line_reader);
+
+    delete [] m_factors_table;


SG_FREE is better. I mean use SG_ALLOC and SG_FREE.

hushell · 2014-04-13T08:23:59Z

@abinashpanda Almost here! Great job! Just two things: 1) merge all commits to a single one, by using git reset --soft; 2) Make sure valgrind reports no memory leaks: valgrind --leak-check=full ./shogun-unit-test --gtest_filter=UAIFileTest*

abinashpanda · 2014-04-13T09:59:35Z

Ran valgrind test and no memory leaks found.
ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0).

tklein23 · 2014-04-13T11:47:55Z

@abinashpanda - Congratulations.

Let's wait for Travis and @hushell to merge. ;)

hushell · 2014-04-13T19:36:15Z

@tklein23 It looks great for me! Please merge for Abinash.

Created CUAIFile class (Issue #1913) to save and load file in UAI file format.

hushell reviewed Mar 17, 2014
View reviewed changes

hushell reviewed Mar 18, 2014
View reviewed changes

vigsterkr reviewed Mar 18, 2014
View reviewed changes

src/shogun/io/UAIFile.cpp

void CUAIFile::parse()

{

Copy link

Member

vigsterkr Mar 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why new line?

vigsterkr reviewed Mar 21, 2014
View reviewed changes

tklein23 reviewed Mar 28, 2014
View reviewed changes

hushell reviewed Mar 30, 2014
View reviewed changes

vigsterkr reviewed Apr 1, 2014
View reviewed changes

tklein23 added the entrance label Apr 3, 2014

abinashpanda closed this Apr 12, 2014

abinashpanda reopened this Apr 12, 2014

hushell reviewed Apr 13, 2014
View reviewed changes

CUAIFile class added to parse and write UAI file format

6b8f833

tklein23 added a commit that referenced this pull request Apr 13, 2014

Merge pull request #2019 from abinashpanda/develop

0ade087

Created CUAIFile class (Issue #1913) to save and load file in UAI file format.

tklein23 merged commit 0ade087 into shogun-toolbox:develop Apr 13, 2014

iglesias mentioned this pull request May 8, 2014

add graphviz plot for decision trees #2195

Closed

Jiaolong mentioned this pull request Jun 6, 2014

UAIFile I/O for factor graph #2283

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created CUAIFile class (Issue #1913) to save and load file in UAI file format. #2019

Created CUAIFile class (Issue #1913) to save and load file in UAI file format. #2019

abinashpanda commented Mar 17, 2014

abinashpanda commented Mar 17, 2014

hushell Mar 17, 2014

hushell commented Mar 17, 2014

hushell commented Mar 18, 2014

hushell Mar 18, 2014

hushell commented Mar 18, 2014

vigsterkr Mar 18, 2014

vigsterkr commented Mar 20, 2014

abinashpanda commented Mar 20, 2014

abinashpanda commented Mar 21, 2014

vigsterkr commented Mar 21, 2014

vigsterkr Mar 21, 2014

abinashpanda commented Mar 21, 2014

hushell commented Mar 21, 2014

tklein23 Mar 28, 2014

vigsterkr Apr 1, 2014

tklein23 commented Mar 28, 2014

abinashpanda commented Mar 29, 2014

tklein23 commented Mar 29, 2014

hushell commented Mar 30, 2014

hushell Mar 30, 2014

tklein23 Mar 30, 2014

hushell commented Mar 30, 2014

vigsterkr Apr 1, 2014

tklein23 commented Apr 11, 2014

abinashpanda commented Apr 12, 2014

hushell commented Apr 12, 2014

karlnapf commented Apr 12, 2014

abinashpanda commented Apr 13, 2014

hushell Apr 13, 2014

hushell commented Apr 13, 2014

abinashpanda commented Apr 13, 2014

tklein23 commented Apr 13, 2014

hushell commented Apr 13, 2014

Created CUAIFile class (Issue #1913) to save and load file in UAI file format. #2019

Created CUAIFile class (Issue #1913) to save and load file in UAI file format. #2019

Conversation

abinashpanda commented Mar 17, 2014

abinashpanda commented Mar 17, 2014

Choose a reason for hiding this comment

hushell commented Mar 17, 2014

hushell commented Mar 18, 2014

Choose a reason for hiding this comment

hushell commented Mar 18, 2014

Choose a reason for hiding this comment

vigsterkr commented Mar 20, 2014

abinashpanda commented Mar 20, 2014

abinashpanda commented Mar 21, 2014

vigsterkr commented Mar 21, 2014

Choose a reason for hiding this comment

abinashpanda commented Mar 21, 2014

hushell commented Mar 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tklein23 commented Mar 28, 2014

abinashpanda commented Mar 29, 2014

tklein23 commented Mar 29, 2014

hushell commented Mar 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hushell commented Mar 30, 2014

Choose a reason for hiding this comment

tklein23 commented Apr 11, 2014

abinashpanda commented Apr 12, 2014

hushell commented Apr 12, 2014

karlnapf commented Apr 12, 2014

abinashpanda commented Apr 13, 2014

Choose a reason for hiding this comment

hushell commented Apr 13, 2014

abinashpanda commented Apr 13, 2014

tklein23 commented Apr 13, 2014

hushell commented Apr 13, 2014