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

Either remove SINDyOptimizer or unbias arg in BaseOptimizer #319

Closed
Jacob-Stevens-Haas opened this issue Apr 24, 2023 · 1 comment · Fixed by #380
Closed

Either remove SINDyOptimizer or unbias arg in BaseOptimizer #319

Jacob-Stevens-Haas opened this issue Apr 24, 2023 · 1 comment · Fixed by #380

Comments

@Jacob-Stevens-Haas
Copy link
Member

Originally posted in #295, where a user (and I) were confused that model coefficients don't agree with optimizer coefficients. This is because SINDy.fit() creates a SINDyOptimizer as a final step to wrap the existing optimizer and unbias the coefficients.

Currently, the SINDyOptimizer class (and file) only exist to do the unbiasing step and does not inherit BaseOptimizer. OTOH, BaseOptimizer mentions an unbias parameter in its docstring but does not actually implement it. Should SINDyOptimizer be removed, and BaseOptimizer incorporate unbias? Or should the docstring for BaseOptimizer remove unbias? I'm slightly in favor of the former: it feels like the correct way to add functionality to all subclasses is to add the functionality to the base class, rather than wrapping all the objects after instantiation. I've vacillated on this issue a few times, however.

@znicolaou
Copy link
Collaborator

Yes, the unbiasing step is currently confusing. I think it would be more natural especially if the opt.history_ of the optimizer opt used in a fit included the last unbiasing step and if the final 'opt.coef_' was actually the unbiased coefficient, which I think doesn't happen currently because the new wrapper object is created around opt. I agree that it would be better to implement it in the BaseOptimizer and remove the SINDyOptimizer wrapper.

@Jacob-Stevens-Haas Jacob-Stevens-Haas changed the title Either remove SINDyOptimizer or unbias arg in BaseOptimizer Either remove SINDyOptimizer or unbias arg in BaseOptimizer Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants