Swap boost::variant with vtable.

[zoq]

I updated the abstract class and also update the Linear layer as an example, there are various layer we have to update, so if anybody likes to work on some of the layers I listed below, comment on the PR. Unfortunately I can't enable commit permission to a specific branch. So to get the changes in you you can just fork the repository as usual create a new feature branch and do the changes, but instead of opening another PR, just post the link to the branch here and I cherry-pick the commit.

Steps:

1. Inherit the Layer class, each layer should implement the necessary functions that are relevant for the layer-specific computations and inherent the rest from the base class.
2. Rename InputDataType to InputType and OutputDataType to OutputType, to make the interface more consistent with the rest of the codebase, rename the type for the input and output data.
3. Use InputType and OutputType instead of arma::mat or arma::Mat<eT>, to make the layer work with the abstract class we have to follow the interface accordingly.
4. Provide default layer type to hide some of the template functionalities that could be confusing for users that aren’t familiar with templates. So instead of using Linear<> all the time, a user can just use Linear. This is a result of "The future of mlpack", round two! #2524 (comment).
5. Update the tests to use the updated interface.

Example: For an example checkout the Linear layer.

Here is a list of layers we have to update:

• adaptive_max_pooling.hpp - @Aakash-kaushik
• adaptive_mean_pooling.hpp - @Aakash-kaushik
• add.hpp - @Aakash-kaushik
• add_merge.hpp - @Aakash-kaushik
• alpha_dropout.hpp - @Aakash-kaushik
• atrous_convolution.hpp - @Aakash-kaushik
• batch_norm.hpp - @Aakash-kaushik
• base_layer.hpp - @mrityunjay-tripathi
• bilinear_interpolation.hpp - @mrityunjay-tripathi
• c_relu.hpp - @zoq
• celu.hpp - @zoq
• concat.hpp - @mrityunjay-tripathi
• concat_performance.hpp - @hello-fri-end
• concatenate.hpp - @mrityunjay-tripathi
• constant.hpp - @zoq
• convolution.hpp - @mrityunjay-tripathi
• dropconnect.hpp - @zoq
• dropout.hpp - @zoq
• elu.hpp - @zoq
• fast_lstm.hpp - @mrityunjay-tripathi
• flexible_relu.hpp - @zoq
• glimpse.hpp - @mrityunjay-tripathi
• gru.hpp - @zoq
• hard_tanh.hpp - @zoq
• hardshrink.hpp - @zoq
• highway.hpp - @mrityunjay-tripathi
• join.hpp - @mrityunjay-tripathi
• layer_norm.hpp - @mrityunjay-tripathi
• leaky_relu.hpp - @zoq
• linear.hpp - @zoq
• linear3d.hpp - @mrityunjay-tripathi
• linear_no_bias.hpp - @zoq
• log_softmax.hpp - @zoq
• lookup.hpp - @mrityunjay-tripathi
• lstm.hpp - @zoq
• max_pooling.hpp - @mrityunjay-tripathi
• mean_pooling.hpp - @mrityunjay-tripathi
• minibatch_discrimination.hpp - @hello-fri-end
• multihead_attention.hpp - @mrityunjay-tripathi
• multiply_constant.hpp - @zoq
• multiply_merge.hpp - @mrityunjay-tripathi
• noisylinear.hpp - @zoq
• padding.hpp - @mrityunjay-tripathi
• parametric_relu.hpp - @zoq
• positional_encoding.hpp - @mrityunjay-tripathi
• radial_basis_function.hpp - @hello-fri-end
• recurrent.hpp - @kaushal07wick
• recurrent_attention.hpp - @kaushal07wick
• reinforce_normal.hpp - @mrityunjay-tripathi
• reparametrization.hpp - @mrityunjay-tripathi
• select.hpp - @mrityunjay-tripathi
• sequential.hpp - @mrityunjay-tripathi
• softmax.hpp - @zoq
• softmin.hpp - @zoq
• softshrink.hpp - @zoq
• spatial_dropout.hpp - @zoq
• subview.hpp - @mrityunjay-tripathi
• transposed_convolution.hpp - @mrityunjay-tripathi
• virtual_batch_norm.hpp - @mrityunjay-tripathi
• vr_class_reward.hpp - @mrityunjay-tripathi
• weight_norm.hpp - @mrityunjay-tripathi

I left the base layer since I'm not sure yet if it makes sense to implement them as an independent class.

---

Building upon the work from @Aakash-kaushik we can get a first impression of the advantage of using boost::visitor compared with a virtual inheritance approach (#2647)

Note we stripped basically everything out, except the FNN class, linear layer, FlexibleReLU layer, LogSoftMax layer; which enables us to get a first impression about what timings we can expect from a virtual inheritance approach.

I tested two scenarios, but used the same network for each:

FFN<> model;
model.Add<Linear<> >(trainData.n_rows, 128);
model.Add<FlexibleReLU<> >();
model.Add<Linear<> >(128, 256);
model.Add<Linear<> >(256, 256);
model.Add<Linear<> >(256, 256);
model.Add<Linear<> >(256, 256);
model.Add<Linear<> >(256, 512);
model.Add<Linear<> >(512, 2048);
model.Add<Linear<> >(2048, 512);
model.Add<Linear<> >(512, 8);
model.Add<Linear<> >(8, 3);
model.Add<LogSoftMax<> >();

Scenario - 1

batch-size: 1
iterations: 10000
trials: 10

vtable - DEBUG=ON

mlpack version: mlpack git-aa6d2b1aa
armadillo version: 9.200.7 (Carpe Noctem)
Filters: FFVanillaNetworkTest
elapsed time: 494.485s
elapsed time: 503.777s
elapsed time: 496.802s
elapsed time: 499.928s
elapsed time: 502.504s
elapsed time: 495.735s
elapsed time: 495.745s
elapsed time: 505.284s
elapsed time: 495.32s
elapsed time: 495.209s
--------------------------------------
elapsed time averaged(10): 498.479s

boost::variant - DEBUG=ON

mlpack version: mlpack git-4d01fe5e9
armadillo version: 9.200.7 (Carpe Noctem)
Filters: FFVanillaNetworkTest
elapsed time: 496.419s
elapsed time: 495.27s
elapsed time: 494.769s
elapsed time: 494.922s
elapsed time: 497.729s
elapsed time: 497.464s
elapsed time: 498.024s
elapsed time: 501.722s
elapsed time: 500.59s
elapsed time: 497.925s
--------------------------------------                                                                                                                                                                                                                                                                                       
elapsed time averaged (10): 497.483s   

vtable - DEBUG=OFF

mlpack version: mlpack git-aa6d2b1aa
armadillo version: 9.200.7 (Carpe Noctem)
Filters: FFVanillaNetworkTest
elapsed time: 199.713s
elapsed time: 205.177s
elapsed time: 200.135s
elapsed time: 200.179s
elapsed time: 205.792s
elapsed time: 198.293s
elapsed time: 198.535s
elapsed time: 206.635s
elapsed time: 198.263s
elapsed time: 198.521s
--------------------------------------
elapsed time averaged(10): 201.124s

boost::variant - DEBUG=OFF

mlpack version: mlpack git-4d01fe5e9
armadillo version: 9.200.7 (Carpe Noctem)
Filters: FFVanillaNetworkTest
elapsed time: 198.645s
elapsed time: 194.854s
elapsed time: 194.748s
elapsed time: 194.983s
elapsed time: 197.42s
elapsed time: 196.864s
elapsed time: 197.454s
elapsed time: 204.318s
elapsed time: 201.076s
elapsed time: 200.549s
--------------------------------------
elapsed time averaged (10): 198.091s

Scenario - 2

batch-size: 32
iterations: 10000
trials: 10

vtable - DEBUG=ON

mlpack version: mlpack git-aa6d2b1aa
armadillo version: 9.200.7 (Carpe Noctem)
Filters: FFVanillaNetworkTest
elapsed time: 70.4116s
elapsed time: 70.5631s
elapsed time: 70.682s
elapsed time: 70.5635s
elapsed time: 71.2245s
elapsed time: 71.1649s
elapsed time: 71.4714s
elapsed time: 71.2688s
elapsed time: 71.3348s
elapsed time: 71.3406s
--------------------------------------
elapsed time averaged(10): 71.0025s

boost::variant - DEBUG=ON

mlpack version: mlpack git-4d01fe5e9
armadillo version: 9.200.7 (Carpe Noctem)
Filters: FFVanillaNetworkTest
elapsed time: 70.3247s
elapsed time: 70.5059s
elapsed time: 70.5368s
elapsed time: 70.5208s
elapsed time: 70.4539s
elapsed time: 70.788s
elapsed time: 70.7692s
elapsed time: 70.9473s
elapsed time: 70.9146s
elapsed time: 70.7278s
--------------------------------------
elapsed time averaged (10): 70.6489s

vtable - DEBUG=OFF

mlpack version: mlpack git-aa6d2b1aa
armadillo version: 9.200.7 (Carpe Noctem)
Filters: FFVanillaNetworkTest
elapsed time: 59.7968s
elapsed time: 59.4626s
elapsed time: 59.9147s
elapsed time: 59.9682s
elapsed time: 60.5511s
elapsed time: 60.2109s
elapsed time: 60.7782s
elapsed time: 60.4981s
elapsed time: 60.719s
elapsed time: 60.7632s
--------------------------------------
elapsed time averaged(10): 60.2663s

boost::variant - DEBUG=OFF

mlpack version: mlpack git-4d01fe5e9
armadillo version: 9.200.7 (Carpe Noctem)
Filters: FFVanillaNetworkTest
elapsed time: 60.8466s
elapsed time: 61.0629s
elapsed time: 61.1269s
elapsed time: 60.7426s
elapsed time: 60.8178s
elapsed time: 60.7287s
elapsed time: 60.864s
elapsed time: 60.8982s
elapsed time: 60.9232s
elapsed time: 60.8519s
--------------------------------------
elapsed time averaged (10): 60.8863s

Looking at the timings, boost::variant doesn't provide the speedup I thought it would, on top of that the little speedup we would gain with boost::variant is marginal in comparison to the actual calculation.

[shrit]

Totally agreed, there is no difference in speed at all in some cases, the mean values are very similar.
We can say that boost::variant and vtables are the same, in addition, we are losing a lot of compilation time with boost that will never happen in vtables.
As a result, boost is adding more disadvantages and reducing the speed if we count the compilation time with the running time.
Thank you @zoq for the comparison 👍

[zoq]

Totally agreed, there is no difference in speed at all in some cases, the mean values are very similar.
We can say that boost::variant and vtables are the same, in addition, we are losing a lot of compilation time with boost that will never happen in vtables.
As a result, boost is adding more disadvantages and reducing the speed if we count the compilation time with the running time.
Thank you @zoq for the comparison 👍

Agreed, don't think it makes sense to stick with boost::variant at this point, the number will slightly change once the vtable gets bigger but I don't think the effect will be huge.

From my side, I can clean-up the interface first so it's easier to collaborate on the PR to incorporate the other layers as well. Unless someone has a strong opinion against the current approach.

[Aakash-kaushik]

Hey this is more of a doubt but can't the Layer class be made abstract by declaring those functions purely virtual which don't actually return anything, that way no object of base class could be initialized and the function which do return something will still act as the base case.

Specifics: talking about layer.hpp and functions like Forward, Backward, Gradient and Reset.

[rcurtin]

Fascinating results! Thanks @zoq and @Aakash-kaushik for making this simulation happen. I agree with the other conclusions here---boost::variant might provide a speedup (I am not sure if it is statistically significant), but even if it does, that speedup is outweighed by the massive slowdown that we think it causes at compile time. It will be a big effort, but I would support (and help) refactoring. I know that we use variant a few other places; specifically for the NSModel, RSModel, RAModel, FastMKSModel, and KDEModel classes. So that will need to be refactored too, and I can handle that part. (There's no need to do any timing simulations there because any function would be only called once or twice per run of a binding. I only used variant there to avoid inheritance, which is a decision I now regret because I unintentionally made more work for now. :))

[Aakash-kaushik]

Also this looks like this might be a lot of work and I would like to help in the refactoring process so let me know if i can help somewhere.

[zoq]

Hey this is more of a doubt but can't the Layer class be made abstract by declaring those functions purely virtual which don't actually return anything, that way no object of base class could be initialized and the function which do return something will still act as the base case.

Specifics: talking about layer.hpp and functions like Forward, Backward, Gradient and Reset.

If we make the functions purely virtual, each layer has to implement the function; right now each layer implements the Forward and Backward method, but the loss layer uses a different interface -> const arma::mat&, const arma::mat& instead of const arma::mat&, arma::mat& so we would have to change the interface of the loss function layers. In addition not every layer implements the Gradient or Reset method (almost every activation layer), so the only function that I think could be purely virtual is the Backward function. So I'm not sure it makes sense to go with a purely virtual method class, what do you think?

[zoq]

Also this looks like this might be a lot of work and I would like to help in the refactoring process so let me know if i can help somewhere.

Absolutely, we do have a bunch of layers so any help would be great. My current plan is to clean-up the interface first and then I can add a list of layers that we have to modify. I will check if I can provide commit rights to my branch, so you could directly push changes.

[Aakash-kaushik]

Hey this is more of a doubt but can't the Layer class be made abstract by declaring those functions purely virtual which don't actually return anything, that way no object of base class could be initialized and the function which do return something will still act as the base case.
Specifics: talking about layer.hpp and functions like Forward, Backward, Gradient and Reset.

If we make the functions purely virtual, each layer has to implement the function; right now each layer implements the Forward and Backward method, but the loss layer uses a different interface -> const arma::mat&, const arma::mat& instead of const arma::mat&, arma::mat& so we would have to change the interface of the loss function layers. In addition not every layer implements the Gradient or Reset method (almost every activation layer), so the only function that I think could be purely virtual is the Backward function. So I'm not sure it makes sense to go with a purely virtual method class, what do you think?

So my main purpose to introduce a purely virtual function in the base class is to ensure that even by mistake a base class object is not created and if someone somehow does that the compiler would throw a very much understandable error, and even if we can just make a single function purely virtual which you suggested to be the Backward function i think that would make the class abstract and achieve the purpose. I am not sure if there are other ways to do this so your feedback would be really great on this.

[Aakash-kaushik]

Also this looks like this might be a lot of work and I would like to help in the refactoring process so let me know if i can help somewhere.

Absolutely, we do have a bunch of layers so any help would be great. My current plan is to clean-up the interface first and then I can add a list of layers that we have to modify. I will check if I can provide commit rights to my branch, so you could directly push changes.

That would be really awesome. Do let me know when you do that.

[zoq]

I'll clean-up and add the remaining methods for the FFN class next.

[zoq]

So my main purpose to introduce a purely virtual function in the base class is to ensure that even by mistake a base class object is not created and if someone somehow does that the compiler would throw a very much understandable error, and even if we can just make a single function purely virtual which you suggested to be the Backward function i think that would make the class abstract and achieve the purpose. I am not sure if there are other ways to do this so your feedback would be really great on this.

Sounds reasonable to me, my suggestion would be to update all the layers first so we know what functions we need, and then we can check which method we can make pure-virtual, maybe we can add a warning or so if a method from the abstract is called.

[Aakash-kaushik]

So my main purpose to introduce a purely virtual function in the base class is to ensure that even by mistake a base class object is not created and if someone somehow does that the compiler would throw a very much understandable error, and even if we can just make a single function purely virtual which you suggested to be the Backward function i think that would make the class abstract and achieve the purpose. I am not sure if there are other ways to do this so your feedback would be really great on this.

Sounds reasonable to me, my suggestion would be to update all the layers first so we know what functions we need, and then we can check which method we can make pure-virtual, maybe we can add a warning or so if a method from the abstract is called.

Sure that seems good to me, also for now i will take up adaptive_max_pooling.hpp and adaptive_mean_pooling.hpp.

[zoq]

So my main purpose to introduce a purely virtual function in the base class is to ensure that even by mistake a base class object is not created and if someone somehow does that the compiler would throw a very much understandable error, and even if we can just make a single function purely virtual which you suggested to be the Backward function i think that would make the class abstract and achieve the purpose. I am not sure if there are other ways to do this so your feedback would be really great on this.

Sounds reasonable to me, my suggestion would be to update all the layers first so we know what functions we need, and then we can check which method we can make pure-virtual, maybe we can add a warning or so if a method from the abstract is called.

Sure that seems good to me, also for now i will take up adaptive_max_pooling.hpp and adaptive_mean_pooling.hpp.

Awesome, updated the issue.

[Aakash-kaushik]

Written mast instead of mask.

[zoq]

Nice catch.

[Aakash-kaushik]

Hey made couple of other changes and also updated the two layers that i took, Can you take a look here.

Next I will be taking:

1. max_pooling.hpp
2. mean_pooling.hpp
3. add.hpp
4. add_merge.hpp
5. alpha_dropout.hpp
6. atrous_convolution.hpp
7. batch_norm.hpp

[zoq]

Hey made couple of other changes and also updated the two layers that i took, Can you take a look here.

Next I will be taking:

1. max_pooling.hpp

2. mean_pooling.hpp

3. add.hpp

4. add_merge.hpp

5. alpha_dropout.hpp

6. atrous_convolution.hpp

7. batch_norm.hpp

Updated the issue, and cherry-picked the commits from your repo.

[Aakash-kaushik]

Hey made couple of other changes and also updated the two layers that i took, Can you take a look here.
Next I will be taking:

1. max_pooling.hpp

2. mean_pooling.hpp

3. add.hpp

4. add_merge.hpp

5. alpha_dropout.hpp

6. atrous_convolution.hpp

7. batch_norm.hpp

Updated the issue, and cherry-picked the commits from your repo.

Hi, Thank you so much, Also i made some specific changes for me such as turning bindings off in cmake and turning profile and debug on by default, I think we can take a look at them at last.

[zoq]

I think I can, just wanted to wait for the merge conflict to get resolved first.

[zoq]

Hm, for some reason the memory job is not working, but I can see we use -DDOWNLOAD_DEPENDENCIES=ON, I'll look into it.

[zoq]

I think I can, just wanted to wait for the merge conflict to get resolved first.

Nevermind, you are right, I can't. But yeah, phenomenal work on this one, super excited to have this one merged.

[rcurtin]

Thanks! Now the real fun begins 😄 (but hopefully with more reasonable diffs!)

[rcurtin]

Maybe mlpack-bot needs an approval with a comment?

[mlpack-bot]

Second approval provided automatically after 24 hours. 👍

[rcurtin]

Ok, in it goes! I'll start opening follow-up issues to be resolved tomorrow.