-
Notifications
You must be signed in to change notification settings - Fork 328
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
[BUG] Optimizers do not obey fit_intercept
= False, or arg is ambiguous
#338
Comments
Just adding two cents here, I think that it may have been a mistake sometime early in development to include the The Most of the |
So what's your preference - do we deal with the coefficient term in the regression, and prohibit constant terms in feature libraries in order to prevent people from including it multiple times? Or do we get rid of |
Adding my random opinions on the various pull requests now: I agree with Zach that the sklearn integration is not only unnecessary bloat, but actively harms future development. For instance, we have been thinking for a while of adding a multi-step prediction error functionality, but this requires using Jax, and somehow squaring all of this with the sklearn integration. Not sure how much effort it would be to minimize the sklearn dependency here. I agree that the two ways of specifying a constant term is not good and I'm happy however we deal with it. |
I've settled on removing the argument from optimizers. It's a much harder task to remove it from all function libraries, and I believe that's where people will expect it (e.g. 0th order polynomials). It incurs a minor performance hit in some cases (because scikitlearn's That seems a reasonable sacrifice in order to prevent the tragic 😭 case of fitting both an intercept and a constant term, which could only otherwise be achieved with higher coupling and more tests. |
Fixes #338 Initialize fit_intercept in super() as False Prevent the _tragic_ 😭 case of fitting both an intercept and a constant term, which could only otherwise be achieved with higher coupling and more tests. Users can specify a constant term in the feature libraries using include_bias, where it exists, or by adding a constant term to a custom library and concatenating. This change incurs a minor performance hit that disappears with scale, based upon how sklearn.LinearRegression fit the coefficient separately from the regression matrices.
Fixes dynamicslab#338 Initialize fit_intercept in super() as False Prevent the _tragic_ 😭 case of fitting both an intercept and a constant term, which could only otherwise be achieved with higher coupling and more tests. Users can specify a constant term in the feature libraries using include_bias, where it exists, or by adding a constant term to a custom library and concatenating. This change incurs a minor performance hit that disappears with scale, based upon how sklearn.LinearRegression fit the coefficient separately from the regression matrices.
Problem
See coefficient plots in example notebooks 1 and 5, which both show the coefficient matrix$\xi$ , fit to an equation of the form:
Expected vs Actual
As I understood it, if$\xi_0$ should always be zero. But it clearly isn't in those examples.
fit_intercept=False
(default)T/S
thefit_intercept
parameter is used to calculate offsets inBaseOptimizer.fit()
. Those results are used later in the function to explicitlyself._set_intercept()
. Might also be some interaction withnormalize_columns
.Nevermind, this is because even if optimizer
fit_intercept
is false,PolynomialLibrary
defaults to providing a zeroeth order term unlessinclude_bias
is false (true by default).WeakPDELibrary
andSINDyPILibrary
also has aninclue_bias
term because they're doing more than just evaluating functions, but maybe that's another piece of evidence that they should be subclasses ofSINDy
, notBaseFeatureLibrary
.It's also unclear how these options interact - i.e. if both are true, does the model try to fit two intercepts? I'm planting a flag that we ought to remove one or both of these options as part of the breaking changes I discussed last week, slightly biased towards removing
include_bias
.The text was updated successfully, but these errors were encountered: