Conversation
|
Okay, I had a systematic problem where I was not passing through I'm finding 1000 is awfully big, that I can do better with a smaller window of 100. Here is the old solution with 1000 -> 100, 200->20, 400->40: 
And here is with the transformation to use the sliding function from the utilities as in this PR: 
Looking identical. Really the window size and stride could be made parameters like they are for things in the linear module. |
pynumdiff/tests/test_diff_methods.py
Outdated
| ttt = np.linspace(0, 3, 201) # for testing jerk_sliding, which requires > 1001 points | ||
| np.random.seed(7) # for repeatability of the test, so we don't get random failures | ||
| noise = 0.05*np.random.randn(*t.shape) | ||
| long_noise = 0.05*np.random.randn(*ttt.shape) |
There was a problem hiding this comment.
I define noise up here before anything runs for consistency. Otherwise I was finding I'd get different answers when running a test on its own versus with everyone else.
There was a problem hiding this comment.
Ended up adding window_size to jerk_sliding so that I could test it with the same t and noise as everyone else.
pynumdiff/tests/test_diff_methods.py
Outdated
| (smooth_acceleration, {'gamma':2, 'window_size':5}), (smooth_acceleration, [2, 5]) | ||
| # TODO (jerk_sliding), because with the test cases here (len < 1000) it would just be a duplicate of jerk | ||
| (smooth_acceleration, {'gamma':2, 'window_size':5}), (smooth_acceleration, [2, 5]), | ||
| (jerk_sliding, {'gamma':1e2, 'solver':'CLARABEL'}), (jerk_sliding, [1e2], {'solver':'CLARABEL'}) |
There was a problem hiding this comment.
The OSQP solver was giving me trouble in the CI again.
| request.config.plots = defaultdict(lambda: pyplot.subplots(6, 2, figsize=(12,7))) # 6 is len(test_funcs_and_derivs) | ||
|
|
||
| def pytest_sessionfinish(session, exitstatus): | ||
| if not hasattr(session.config, 'plots'): return |
There was a problem hiding this comment.
Occasionally I'd get a failure while testing about this not being able to plot when that wasn't at all relevant. The cause was always a syntax error in the test code file, but this would obfuscate that with a different confusing error. Now we fall through to highlight the syntax error.
| from ..finite_difference import first_order, second_order | ||
| # Function aliases for testing cases where parameters change the behavior in a big way | ||
| def iterated_first_order(*args, **kwargs): return first_order(*args, **kwargs) | ||
|
|
There was a problem hiding this comment.
I briefly had some other vectors up here, but switching between them in the test is annoying, so I've tried to keep everyone on this one t and dt.
| kernel = utility.gaussian_kernel(9) | ||
|
|
||
| x_hat, dxdt_hat = utility.slide_function(identity, x, 0.1, kernel, step_size=2) | ||
| x_hat, dxdt_hat = utility.slide_function(identity, x, 0.1, kernel, stride=2) |
There was a problem hiding this comment.
I decided to rename this parameter. I like stride better, because dt is a step size. Less ambiguous.
| dxdt_hat = y/dt # y only holds the dx values; to get deriv scale by dt | ||
| x_hat = np.cumsum(y) + integration_constants.value[order-1] # smoothed data | ||
|
|
||
| dxdt_hat = (dxdt_hat[0:-1] + dxdt_hat[1:])/2 # take first order FD to smooth a touch |
There was a problem hiding this comment.
No need for that little 0.
|
|
||
|
|
||
| def jerk_sliding(x, dt, params=None, options=None, gamma=None, solver=None): | ||
| def jerk_sliding(x, dt, params=None, options=None, gamma=None, solver=None, window_size=101): |
There was a problem hiding this comment.
I've added the window_size parameter to match methods from the linear module. I have not, however, added the stride/step_size. We could, but I've just kept to striding by the ramp width.
| gamma = params[0] if isinstance(params, list) else params | ||
| if options != None: | ||
| if 'solver' in options: solver = options['solver'] | ||
| if 'window_size' in options: window_size = options['window_size'] |
There was a problem hiding this comment.
I've allowed users to pass this via the old way too, just so I can be consistent in my tests.
| raise ValueError("`gamma` must be given.") | ||
|
|
||
| window_size = 1000 | ||
| stride = 200 |
There was a problem hiding this comment.
Hard-coding these makes this method less usable.
| from pynumdiff.finite_difference import second_order | ||
| return second_order(x, dt) | ||
| ramp = window_size//5 | ||
| kernel = np.hstack((np.arange(1, ramp+1)/ramp, np.ones(window_size - 2*ramp), (np.arange(1, ramp+1)/ramp)[::-1])) |
There was a problem hiding this comment.
This kernel isn't exactly the same as before, but it's basically the same shape, the same in spirit.
|
Okay, based on the identicalness of before-and-after and the correctness of the tests, I'm saying this one is a correct PR. I've added to the interface to make this thing easier to test and more flexible to use. Happy with it. Going to act unilaterally again and say this is a positive change and merge. |
1 participant
Attempting to use
slide_functionwithjerk_slidingin the spirit of #69.Essentially the old code was using these kernels
which when normalized get weighted like this:
(This isn't exactly the same as with the
slide_function, because it starts with the kernel half-in the domain and goes until it's half-out of the domain, so you always have more than one solution contributing to a region. But that figure looked cool.)