-
Notifications
You must be signed in to change notification settings - Fork 321
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
Expanded derivatives #85
Conversation
Codecov Report
@@ Coverage Diff @@
## master #85 +/- ##
==========================================
+ Coverage 95.35% 95.37% +0.02%
==========================================
Files 18 19 +1
Lines 968 995 +27
==========================================
+ Hits 923 949 +26
- Misses 45 46 +1
Continue to review full report at Codecov.
|
I believe Not sure which functionality is desired, but I think if the pipe goes to finite difference the methods should be equivalent up to the numerical error of the finite difference method. (If you pipe into a method with additional fits/regularization, they will be different). |
I am slightly worried about the maintenance overhead this implementation causes. Every change in the derivative package requires us to adopt these changes in pysindy too. I'd prefer to use the |
@Ohjeah, I see what you mean and I agree that the code would be more maintainable if we were to use
I'm not sure I understand what this solution would look like. Are you envisioning that rather than specify a class model = SINDy(differentiation_method=SpectralDerivative(...)) the user would supply some keywords? model = SINDy(derivative_kwargs={...}) Or maybe we could write a simple wrapper class for all optimizer = SINDyOptimizer(self.optimizer, unbias=unbias) Or were you thinking something along the lines of having the user pass in a I'm not sure it's good enough reason to outweigh the maintainability concern you raised, but the nice thing about the current implementation is that it allows parameters of the differentiators to be included when performing sklearn-style cross-validation. @andgoldschmidt, I think Once you've updated |
I do agree that a wrapper for PySINDy is important, especially because
is a great feature--honestly essential for users to truly integrate some of these derivative methods into the PySINDy framework. Also, a wrapper let's you create default args if that's appealing. Let me know if
|
I agree with you both. Wrapping is required to ensure the cross-validation functionality and maybe specific input validation and error handling. The wrapping should be flexible enough though to seamlessly upgrade the |
Derivative wrapperI'll take a stab at implementing a universal wrapper for Smoothed finite differencesThe main reason I included this method in the initial release was because it was one that Floris highlighted as being relatively simple, effective, and easy to implement. I agree that we could just leave it to the user to smooth their data before feeding it to a |
@briandesilva You can run a gridsearch on from sklearn.base import BaseEstimator
class Model(BaseEstimator):
def __init__(self, nested_kwargs=None):
self.kwargs = {"nested_kwargs": nested_kwargs}
def fit(self, x, y=None, **fit_params):
print(self.kwargs)
return self
def score(self, x, y):
return 0
def set_params(self, **params):
"""Set the parameters of this estimator.
Modification of the sklearn method to allow unknown kwargs. This allows using
the full range of xgboost parameters that are not defined as member variables
in sklearn grid search.
Returns
-------
self
"""
if not params:
# Simple optimization to gain speed (inspect is slow)
return self
for key, value in params.items():
if hasattr(self, key):
setattr(self, key, value)
else:
self.kwargs[key] = value
return self
import numpy as np
from sklearn.model_selection import train_test_split
from sklearn import datasets
from sklearn import svm
X, y = datasets.load_iris(return_X_y=True)
X_train, X_test, y_train, y_test = train_test_split(X, y, test_size=0.4, random_state=0)
from sklearn.model_selection import GridSearchCV
m = Model()
p = {"nested_kwargs": [{"a": 1}, {"a": 2}]}
grid = GridSearchCV(m, p)
print(grid)
grid.fit(X, y) Model would be the derivative wrapper and |
Okay, I finally got around to updating the implementation based on @Ohjeah's suggestion. I added unit tests and updated the derivative and sklearn example notebooks. The solution ended up being both lightweight and flexible enough to account for changes in the |
LGTM, we should also use an inter-sphinx link to the derivative.py documentation. |
Okay, I'm going to go ahead and merge this and create a new release. |
Expanded derivatives
Expands numerical differentiation capabilities by wrapping the methods in the derivative package. The PySINDy versions of each method have the same names as the objects in
derivative
, but with "Differentiator" appended to the end. I created an example notebook comparing all the differentiation options with and without noise. I also added some basic unit tests, but didn't make them stringent becausederivative
already has its own suite of tests.For now I decided to leave in our original
FiniteDifference
andSmoothedFiniteDifference
methods becausederivative
that implements smoothed finite differences (apply smoothing, then apply a finite difference method)See #58 for more context.
I'm curious whether @andgoldschmidt and @Ohjeah have any thoughts about this implementation.