neuroevolution-ai / NeuroEvolution-CTRNN_new

MIT License
3 stars 3 forks source link

Restructure hierarchy of PyTorch brains (and brains in general) #76

Open pdeubel opened 3 years ago

pdeubel commented 3 years ago

Our current brain hierarchy of the PyTorch brains is a little bit cluttered. While the PyTorch implementations of Elman, GRU and LSTM use IPytorchBrain as a base class, the FFNN PyTorch implementation does not. It would be good if we either

I would opt for the first option because it seems more intuitive to me to have brains from a similiar category grouped.

This issue can also be used to sum TODOs for refactoring that fall into this refactoring task:

DanielZim commented 3 years ago

From both options I would also favor the first one.

In addition, maybe we don't even need the PyTorch brains for the neuroevolutionary training at all, since the numpy versions a) are generally significantly faster (I will measure this more in detail) and b) produce the same outputs than the PyTorch networks. I agree that the PyTorch brains are very important for our tests, but maybe we can remove them from the brain classes. At least, I never used PyTorch brains for training. I think, this would help us keeping our brain class structure clean and simple. Nevertheless, we still provide a high flexibility since users can always implement and use any brain they want by just deriving from the IBrain class and implementing the corresponding functions.

Dranero commented 3 years ago

When we erase the PyTorch brains out of the Project, we also don't need Netwok-specific base classes like FeedForwardBase, because there will only be one subclass of each base class.

So when we remove the PyTorch Brains, option one seems not like a good option.

But even if we keep the PyTorch networks, i prefere option two over one, because it combines way more code, then option one, because as state now there is a lot of code all PyTorch Networks need, but the NumPy and PyTorch versions already differ in many ways (e.g. doubble bias use and layer construction in the config file)

My favorite option would be to chose a network and a package base in the config file and the network would generate itself by combining the methods from the network base class (e.g. how many gates are needed in each neuron) with the methods in the package base class (e.g. how to apply the individual values) But i dont know a suitable way to implement this in the current framework

pdeubel commented 3 years ago

My favorite option would be to chose a network and a package base in the config file and the network would generate itself by combining the methods from the network base class (e.g. how many gates are needed in each neuron) with the methods in the package base class (e.g. how to apply the individual values) But i dont know a suitable way to implement this in the current framework

This sounds and would be really nice but I don't think the effort would be justified. I think we would need to have a class structure something like NumPy -> Linear / Recurrent -> FFNN / {LSTM, GRU, ...}, and the same for PyTorch. Then we have CTRNN which is not implemented in PyTorch and CNN which is not implemented in NumPy etc.

We could remove the PyTorch brains. I personally used them when I originally implemented the LSTM in NumPy to verify that my implementation is correct and I though I keep it because the PyTorch version could be used to move it to the GPU (at least in theory).

If we remove the PyTorch brains then we could rename ILayerBasedBrain to RecurrentBasedBrain and fuse FeedForwardNumpy into one class called FeedForward. CNN (and CnnCtrnn) would then be the only PyTorch implementations (because of the convolutional implementation), but that is okay because they are derived directly from IBrain and I think we measured at some point that a convolution implemented in NumPy is not realy a performance boost over PyTorch and way to much effort to make things like padding etc. to work in the right way.

If we want to keep the PyTorch brains we could still rename ILayerBasedBrain to RecurrentBasedBrain, make FeedForwardPyTorch a subclass of IPytorchBrain and fuse FeedForward and FeedForwardNumPy into a new class FeedForward directly derived from IBrain. This would be at first counterintuitive but it reduces duplicated code.

I would favor for keeping the PyTorch brains, since we already implemented them they may come in handy sometime. We could introduce new files/sub-directories to better separate brains in the file hierarchy which may help to improve the overview on all brains.


Overview on some possibilities:

Current Structure:

image

Remove PyTorch brains:

image

Keep PyTorch brains:

image