-
Notifications
You must be signed in to change notification settings - Fork 8
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
Pass or store smoothed coordinate values #13
Comments
I agree: Looking over the existing methods, I didn't see any case where the derivative was computed from an implicit
The takeaway is that whether global or local, we want to compute As for implementation, I think we want to modify the contract for
Looks to me like option 2 is better. |
I understand that the wrapper in def dxdt(x, t, kind, axis, **kwargs):
method = gen_method(kind, x, t, axis)
return method.d(x, t, axis=axis) # if option 2, then method.d(x,t,axis)[1]
# new function
def smooth_x(x, t, kind, axis, **kwargs):
method = gen_method(kind, x, t, axis)
return method.x(x, t, axis=axis) # if option 2, then method.d(x,t,axis)[0]
@lru_cache
def gen_method(kind, x, t, axis, **kwargs):
return methods.get(kind)(**kwargs) The cache means that calling There's probably a pattern/cacheing we could use that keeps |
Ok, so that was much harder than expected, and I ended up implementing not just cacheing for Arrays, but also having to implement reference counting and garbage collection inside the cache. Ended up throwing 90% of it out with a simpler but just as effective solution. The benefit is that we can memoize ALL calls to the expensive methods, e.g. I'm going to add a draft PR with the changes. This follows option (1) above and doesn't even modify the contract for My thoughts on backwards compatibility: We can certainly modify the contracts for |
Some derivative methods smooth the coordinate points while calculating the derivatives, and these smoothed coordinates can be useful for SINDy. I think the list of methods that smooth coordinates are:
There's several options for the interface, either
x, x_dot
fromDerivative.compute_for()
andDerivative.d()
, and possiblydxdt()
.Derivative.x
set when callingDerivative.compute_for()
. Subclasses that do not smoothx
should probably still set the value to maintain interface. Could be set inDerivative._global()
, but not sure how to deal with savitsky golay, since local methods don't need to compute the derivative for all points.As I see it, the pros/cons boil down to explicit is better than implicit for the functional pattern vs backwards compatibility for the object-oriented pattern. Setting a new object attribute after initialization is implicit, changing return type is explicit. The object oriented pattern requires derivative objects remain public, and users like pysindy would need a more complicated calling convention.
in favor of
That said, backwards compatibility is always important. I'm happy to build a PR of either.
The text was updated successfully, but these errors were encountered: