-
Notifications
You must be signed in to change notification settings - Fork 175
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
Corrected regularization and rmse in non-negative solvers #839
Conversation
This doesn't effect the standard solver, does it? |
Nope. It just makes the non-negative solvers comparable in regularization/rmse to the standard solver. |
@@ -19,6 +19,11 @@ | |||
logger = logging.getLogger(__name__) | |||
|
|||
|
|||
def _rmses(A, Y, X): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but I would prefer the order of arguments here to be A, X, Y or Y, A, X, probably the first since the system is typically written AX = Y.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Made suggested changes. |
This looks good to me. |
Me too, just needs some squashin'. You want to merge @hunse? |
sure |
Previously, the
'rmses
' for the non-negative solvers were off by an order of magnitude, because they were with respect to the Gram system instead of the original system. This has been fixed to make these values now comparable to the other solvers.This fix has been tested offline using
nengo.utils.connection.eval_point_decoding
on the same set of eval points (now gives an exact match). If desired, I could also add a unit test to check that each solver's rmse is correct with respect to the given system.Also, the regularization wasn't scaled consistently with the other solvers. This was causing drastically different performance results when using one solver over another (I also wonder if this contributed in part to the performance improvement in #321). I verified the fix (again offline) by comparing the RMSE when decoding a positive constant function -- they were roughly the same across a variety of regularization coefficients. Again, this can be made into a unit test if desired.