Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FFN serialization #516

Closed

Conversation

stereomatchingkiss
Copy link
Contributor

Let FFN able to serialize by boost serialization

Haven't finished yet, there are others parts need to serialize(initialize rules and others) before ffn serialization could be done.

Besides, I cannot serialize arma::cube, finding a solution

@@ -2,7 +2,7 @@
* @file sparse_bias_layer.hpp
* @author Tham Ngap Wei
*
* Definition of the SparseBiasLayer class.
* Definition of the BiasLayer class.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is a documentation mistake?

@rcurtin
Copy link
Member

rcurtin commented Feb 5, 2016

Looks good to me so far, but I only took a quick look. You might find some of the stuff in src/mlpack/tests/serialization.hpp to be helpful for testing. Usually when I test serialization, I'll serialize using text, XML, and binary archives, then make sure all of the models are the same after serialization. Sometimes it's also useful to serialize over an existing model (i.e. make models A and B, then serialize A into a file, then read B from that file), so as to make sure a model properly cleans up its own memory during serialization.

@stereomatchingkiss
Copy link
Contributor Author

Thanks for reviews. I will fix the document mistakes on next commit.

I will write some test cases too, after ffn is done, cnn and rnn should be quite easy to serialize too.

mlpack 2.x still rely on libxml? Could we just use boost to replace it?

@zoq
Copy link
Member

zoq commented Feb 11, 2016

The modified optimizer looks good, thanks for the effort. Once we merge the altered optimizer interface, we have to modify all layer that uses the optimizer interface. If you need some help with this step, don't hesitate to ask.

mlpack 2.x still rely on libxml? Could we just use boost to replace it?

You are right since mlpack 2.0.0 the libxml2 dependency was replaced with boost::serialization.

@zoq zoq mentioned this pull request Feb 11, 2016
@theSundayProgrammer
Copy link
Contributor

Does it really make sense to save input, output, delta or any other variable that is used only during training? The aim of archiving, I think, should be to save the weights and other variables that are needed to deploy "learned machine." Even if we are going to restart the training, variables used exclusively in training are re-initialised anyway.

@rcurtin
Copy link
Member

rcurtin commented Feb 11, 2016

My thoughts here don't apply to this situation in particular (I haven't really looked over this), but I think that in general, serializing user preferences for training is worth doing. For instance, a user may want to load a model, and then train it some more. Any intermediate training variables probably aren't worth saving, but something like a learning rate is worth holding onto, in my opinion. This is what I've tried to do with other serialization code. I hope these thoughts are helpful. :)

@zoq
Copy link
Member

zoq commented Feb 11, 2016

Joseph is right, we don't have to save the input, output and gradient parameter. We don't need the parameter for optimizing an already trained model nor for testing.

@stereomatchingkiss
Copy link
Contributor Author

Thanks for pointing out the defects, I will remove those intermediate training variables on next commit. I would keep those parameters needed for retraining, sometimes I need to reload the weights and use them to finetune the results.

@theSundayProgrammer
Copy link
Contributor

Hi @stereomatchingkiss
I have developed an archive solution. Chekout
http://github.com/theSundayProgrammer/mlpack, 'ann_archive' branch. I have
not made a pull request because I wanted to see what you were doing and my
changes are based on top of the copy constructor modifications. What is
the custom if two developers are working on the same feature in an open
source project?

Also I did not find code to archive a tuple in MLPack code base. So I wrote
one as you will be able to see (core/util/tuple_serialize.hpp). You are
welcome to use it if you wish.

On Fri, Feb 12, 2016 at 10:51 AM, stereomatchingkiss <
notifications@github.com> wrote:

Thanks for pointing out the defects, I will remove those intermediate
training variables on next commit.


Reply to this email directly or view it on GitHub
#516 (comment).

Joseph Chakravarti Mariadassou
http://thesundayprogrammer.com

@stereomatchingkiss
Copy link
Contributor Author

Hi, @ theSundayProgrammer

I think your copy constructor implementation are quite cool, it lower down the dependency between layer and optimizer, make codes become easier to read and serialize. If you already implement the serialization of ann based on it, please open a PR. I will delete my pull request #516 after that. Thanks for your helps :).

ps : If you think there are something can reuse, I will keep it alive before you finish the serialization of the ann.

@zoq
Copy link
Member

zoq commented Feb 24, 2016

I implemented the serialization in 69f97e5, f6c27ed and 43f0f13 for the FFN, RNN and CNN class. Since we removed the optimizer from each layer, we don't have to struggle with the optimizer serialization anymore.

I merge the serialization code, once we removed the parameters like delta, input parameter, output parameter, etc. as Joseph pointed out.

So maybe you can open another pull request? If not I'll go and extract the code from this pull request and make the necessary changes.

As always, please comment if something doesn't make sense and I'll reopen it.

@zoq zoq closed this Feb 24, 2016
@stereomatchingkiss stereomatchingkiss deleted the ffn_serialization branch February 25, 2016 04:16
@stereomatchingkiss
Copy link
Contributor Author

Ok, I will open another pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants