Add Thiele's interpolation formula #24109
Conversation
|
✅ Hi, I am the SymPy bot (v169). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like: This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.12. Click here to see the pull request description that was parsed. |
sympy/polys/rationaltools.py
Outdated
| >>> f.subs(x, 0.5) * 3 | ||
| 3.14159265358979 | ||
|
|
||
| >>> thiele([1, 2, 3, 4], [10, 12, 14, 16]) |
There was a problem hiding this comment.
Would it be better to raise an exception in this case?
There was a problem hiding this comment.
Yes, but how can I achieve that? Not sure about the cleanest wai...
There was a problem hiding this comment.
@asmeurer I tried to implement it. Did I do it the right way with expr.has(nan, zoo)?
There was a problem hiding this comment.
@asmeurer Could you review my code and tell me if it is OK?
|
Please add tests for this. |
What do you mean? Tests for the error cases? How could I do? |
|
@baruchel take a look at a few of the other (merged) pull requests, we require that all new functions are covered by unit tests. |
|
I'm going to mark this as draft. Feel free to set it as ready for review when some tests have been added. |
|
@oscarbenjamin Sorry for the long delay, I added some more tests. Is it OK. Some checks were not successful above, but I can't figure out if it comes from me or from something else in the checking process. |
|
I'm not sure what caused the doctests to fail. I've restarted them. |
sympy/polys/rationaltools.py
Outdated
| """ | ||
| n = len(u) | ||
| assert len(v) == n | ||
| seterr(divide=True) |
There was a problem hiding this comment.
Oh actually this is the problem.
|
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[41d90958] [857a97e6]
<sympy-1.11.1^0>
- 1.11±0.02ms 695±3μs 0.62 solve.TimeSparseSystem.time_linear_eq_to_matrix(10)
- 3.24±0.1ms 1.30±0.02ms 0.40 solve.TimeSparseSystem.time_linear_eq_to_matrix(20)
- 6.55±0.06ms 1.87±0.04ms 0.29 solve.TimeSparseSystem.time_linear_eq_to_matrix(30)
Full benchmark results can be found as artifacts in GitHub Actions |
|
Hi, @oscarbenjamin Is it OK now? Regards. |
|
@baruchel I would expect to find a method named "test_thiele_interpolate" here: |
|
Hi @oscarbenjamin I added some tests. |
| and their function values in vector v (both vectors must be of equal | ||
| lengths). | ||
|
|
||
| This algorithm might encounter division by zero |
There was a problem hiding this comment.
What is an example of this (or reference)?
There was a problem hiding this comment.
A ZeroDivisionError is yielded whenever values are equally spaced. For instance,
thiele([0,1,2,3], [0,1,2,3])
There was a problem hiding this comment.
I would like to see a better criteria stated. If you interpolate on ([1+z,2,3,4],(1,2,3,5)) and then replace z with 0 you get x which is clearly not right. If you use only 3 points as in (1,2,3),(1,2,3) it doesn't fail. Maybe this should be limited to numeric input whose output is unambiguously ok or is an error indicating problems.
There was a problem hiding this comment.
Is the criteria that there cannot be 3 colinear points? There is a routine for checking colinearity in geometry.
|
|
||
| This algorithm might encounter division by zero | ||
| when values are equally spaced. For such cases, a better algorithm is | ||
| sympy.polys.polyfuncs.rational_interpolate. |
There was a problem hiding this comment.
Is it always true that the rational_interpolate(dat, n) == thiele_interpolate(*dat) if n is the degree of the Thiele numerator?
Brief description of what is fixed or changed
Added a new interpolation formula
Release Notes