Added pchip Interpolator #4871
Added pchip Interpolator #4871
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4871 +/- ##
========================================
Coverage 98.70% 98.70%
========================================
Files 304 304
Lines 23452 23483 +31
========================================
+ Hits 23149 23180 +31
Misses 303 303 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for the nice work on this @Rishab87! I added a few comments
| @@ -281,3 +282,19 @@ def test_voltage_without_directions(self): | |||
|
|
|||
| voltage = solution["Terminal voltage [V]"].entries | |||
| assert np.allclose(voltage, 2.5, atol=1e-3) | |||
|
|
|||
| def test_pchip_interpolation_experiment(self): | |||
There was a problem hiding this comment.
Can you please add a few more tests:
- Comparing the results with
scipy.interpolate.PchipInterpolator - Check non-uniform
xgrids - Check that it throws an error for non-increasing
xinputs - Check left/right extrapolation
| ) | ||
| x_np = np.array(symbol.x[0]) | ||
| y_np = np.array(symbol.y) | ||
| pchip_interp = interpolate.PchipInterpolator(x_np, y_np, axis=0) |
There was a problem hiding this comment.
I think the axis=0 argument is unnecessary because of the upstream check for 1D interpolation, but I could be wrong
There was a problem hiding this comment.
yeah its not required
| + h11 * h_val_mx * d1 | ||
| ) | ||
|
|
||
| cond = casadi.logic_and(x_sym >= x0, x_sym <= x1) |
There was a problem hiding this comment.
If we try to extrapolate outside the x bounds, this will return the value defined by
result = casadi.MX.zeros(x_sym.shape)which can be problematic during simulation. Can you please add extrapolation that aligns with scipy's?
There was a problem hiding this comment.
I've added extrapolation aligning with scipy
| d0 = casadi.MX(d_np[i]) | ||
| d1 = casadi.MX(d_np[i + 1]) | ||
|
|
||
| t = (x_sym - x0) / h_val_mx |
There was a problem hiding this comment.
This is minor, but can we keep a consistent notation with either t or x but not both?
There was a problem hiding this comment.
changed this to use x
| x_np = np.array(symbol.x[0]) | ||
| y_np = np.array(symbol.y) | ||
| pchip_interp = interpolate.PchipInterpolator(x_np, y_np, axis=0) | ||
| d_np = pchip_interp.derivative()(x_np) |
There was a problem hiding this comment.
Can we access this array directly from the properties of the pchip class?
There was a problem hiding this comment.
yes i think so as protected attribute
|
@MarcBerliner thanks for your review, I've added made changes according to your comments |
There was a problem hiding this comment.
Thanks @Rishab87, this is great. I just added one minor changelog update, which I missed the first time around. Otherwise, it's good to go
Co-authored-by: Marc Berliner <[email protected]>
|
@MarcBerliner I've applied that change! |
Description
Added pchip Interpolator support in PyBaMM
Fixes #4590
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)
Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests