Use index guessing functionality moved to FindFirstFunctions.jl (Guesser)
#323
Conversation
SouthEndMusic
changed the title
f = +/-Infget_idx when f = +/-Inf
|
Update: will use the updated version of |
SouthEndMusic
changed the title
get_idx when f = +/-InfFindFirstFunctions.jl (``)
SouthEndMusic
changed the title
FindFirstFunctions.jl (``)FindFirstFunctions.jl (Guesser)
src/derivatives.jl
Outdated
| @@ -1,16 +1,11 @@ | |||
| function derivative(A, t, order = 1) | |||
| function derivative(A, t, order = 1; iguess = A.iguesser) | |||
There was a problem hiding this comment.
why do we need iguess as a kwarg here?
There was a problem hiding this comment.
To support a user-supplied iguess:
There was a problem hiding this comment.
Ah wait no that's only for interpolation, shouldn't we support that for derivatives too like this?
There was a problem hiding this comment.
But in that case it should be documented
There was a problem hiding this comment.
As the time steps is the same, we should just use iguesser from interpolation objects.
There was a problem hiding this comment.
I don't think that would be worth it. IMO, users should not give iguess themselves for each derivative/interpolation computation (not the construction).
There was a problem hiding this comment.
Ok, but you want to keep https://github.com/SouthEndMusic/DataInterpolations.jl/blob/3debd2909676a0c315ee41a76b90fe9ab146d6e8/src/DataInterpolations.jl#L25-L27 (as it is already there)? To me it feels inconsistent to have it for interpolations but not for derivatives
There was a problem hiding this comment.
I think we can remove it as it is not documented.
There was a problem hiding this comment.
Ok, thanks for taking the time to discuss this 👍
There was a problem hiding this comment.
Turns out it is not tested either
|
@thazhemadam is there some issue with the cache? I'm going to just merge. |
|
I think it might be a transient issue. I don't see it happening on |
Fix #322.