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

Add support for keras models for outputs #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jonahweissman
Copy link

@jonahweissman jonahweissman commented Jul 17, 2018

To follow up on #5, here's what I worked out: Keras layer building turned out to be too much of a challenge for me, so I modified pyESN to optionally accept a Keras model as an input. Now, instead of learning linear weights to map readouts to outputs, it trains the Keras model to do so. If you don't think that this is appropriate to be included in the main code, that's totally fine. I just wanted to share what I did.

Here's mackey.ipynb, modified to use my new functionality. The performance is worse, but it shows how it works. (GitHub won't let me upload .ipynb, so I changed the file extension to .txt. Just change it back to view.)

I added a new test, but it would only catch the most extreme errors. I wasn't sure of what to include in a more useful test. The freq_gen performance test fails both before and after my changes.

Also, I'd like to add a .gitignore. Should I open a new PR or can I just add it to this one?

@jonahweissman
Copy link
Author

I fixed the bug that was causing the freq_gen performance bug. The activation function is applied to the data twice in ESN.predict; once while stepping through states and again to all the outputs before they are returned. I had gotten rid of the second time, but I've added it back in dd03086.

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