colgreen / sharpneat

SharpNEAT - Evolution of Neural Networks. A C# .NET Framework.
https://sharpneat.sourceforge.io/
Other
372 stars 95 forks source link

GetActivationFunction(string) in NetworkXmlIO.cs unable to load existing run with a custom activation function #39

Closed naigonakoii closed 5 years ago

naigonakoii commented 5 years ago

I have the following code in my main function:

IActivationFunction activation = new SigmoidActivation(); NeatGenomeParameters genomeParams = new NeatGenomeParameters { ActivationFn = activation, }; NeatGenomeFactory genomeFactory = new NeatGenomeFactory(8, 1, genomeParams);

Which works fine for starting a new run. If I then save the run xml:

var doc = NeatGenomeXmlIO.SaveComplete(neat.GenomeList, nodeFnIds: true); doc.Save("d:\\neat\\generation.xml");

The xml is saved as expected. However, loading the xml crashes because GetActivationFunction(string) throws since it only maintains a hard-coded list.

naigonakoii commented 5 years ago

I have a proposed fix but git won't allow me permissions to publish my branch to the server. If you make me a contributor will that allow me to publish it? Or is there another way to publish a branch?

colgreen commented 5 years ago

Hi,

Or is there another way to publish a branch?

The standard git/github way is that you:

1) create a fork of sharpneat/master (click the fork button on the sharpneat github page). 2) Checkout that fork to your PC and makes changes there. 3) Commit your changes to your fork on github. 4) Issue a pull request from your branch to mine, containing just the commits you want to push back to sharpneat/master.

I review he changes and accept/reject the pull request. If it's a small change I might just apply the change manually to sharpneat/master, so if it is small then just post the code as a comment here and I will review it.

naigonakoii commented 5 years ago

Merge request created:

https://github.com/colgreen/sharpneat/pull/40

colgreen commented 5 years ago

Thanks. This now merged. I made a few changes, the main one is that you will need to make the call to register your activation function instead of it being called from the constructor of NeatGenomeFactory. I felt like that wasn't quite the right place for that.

Also, I haven't changed this, but for the record - it's nearly always a bad idea to put state like this in static variables, e.g. if someone was to run multiple neat algorithms in parallel in the same process then they would have a single shared copy of the activation functions dictionary; no a big problem but I'd put it under the heading of 'code smell'. However, the refactor branch will have nice patterns for all this so not too concerned about this.

naigonakoii commented 5 years ago

Cool glad it helped out. My first commit had it so you needed to call the register method publically as well, but that's just another API that the user needs to learn which is why I moved it into the constructor. But I can see an argument for either case, and it's not a big deal to have one extra external call.