-
Notifications
You must be signed in to change notification settings - Fork 2
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
DataInterpolations
support
#178
DataInterpolations
support
#178
Conversation
Is this the way to go with dispatching on callable structs? Or should it be done per interpolation type separately? |
Wow, this PR came impressively quickly after merging #177!
I'm afraid this might just overload the constructors. SCT.overload_gradient_1_to_1(:DataInterpolations, AbstractInterpolation) which will most likely return a function similar to function DataInterpolations.AbstractInterpolation(t::SparseConnectivityTracer.GradientTracer)
return SparseConnectivityTracer.gradient_tracer_1_to_1(t, false)
end which unfortunately is not what you want. I'll try to figure out how to work around it. The simplest solution would be to add a new specialized |
There must be an easier way to get all the interpolation types. Also, for some reason the tests cannot find Edit: nvm, I'll try some different things. |
I think your initial attempt was the intuitive way to do it, it just needs some work from our side. |
I think my latest approach is the best, this is also the level on which other
The original methods are e.g. |
Yes, this kind of function requires writing manual overloads. I should add to the documentation that our "N-to-M operators" assume Judging by its name, Overloading on callable |
For my application I only need scalar to scalar, but for many interpolation types the output can also be a vector. Or is that still considered 1 to 1? |
Are interpolations generic functions An overly conservative estimate could be obtained by using tools from
(This is how we handle matrix inversion for example.) |
|
Could you rebase this PR on I’ll add the needed overloads for you tomorrow. Any tests you add will help me out greatly. |
In general what I think should be supported (maybe each can be a separate issue/PR):
|
Thanks for the list! I think that should all be doable in here. |
By the way, what's the philosophy of where extensions are located? |
I'd say we should have them in here for now:
|
fe318ef
to
598fe2e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
- Coverage 91.36% 90.42% -0.94%
==========================================
Files 41 44 +3
Lines 1772 2037 +265
==========================================
+ Hits 1619 1842 +223
- Misses 153 195 +42 ☔ View full report in Codecov by Sentry. |
1D interpolations should be working now. |
for other package extensions to copy.
Urgh, I wasn't aware DataInterpolations had separate methods for |
Wait, this is an entirely different use-case than what we are currently implementing. You don't want to differentiate through the interpolation, you want to differentiate through the creation of the interpolant? |
I would say that with the additional overloads on tracers, this is unintended behavior, as this PR only returns the input indices of the "interpolation query tracers |
The differentiation of the result of an interpolation w.r.t. to the input arguments of its constructor is also a bit more nuanced than what we are currently doing. |
As usual, I assume that this works out of the box for local tracers, and that only global tracers are an issue?
IIUC this would require constructing each field of the corresponding Of course there's also the crazy option: make |
Maybe before this PR, but with the new overloads we possibly return the wrong patterns.
I‘m not even sure whether there is a way for Global tracers to return any kind of sparsity. Let’s take |
If a dense pattern is the correct solution for global tracers, the solution might not be too complicated: Collect all tracers from the data (if there are any) and take a union over all of them. Then take a union with the „query tracers“ and return a |
You're right about that, sparsity of an interpolation call w.r.t. the input
That is fine by me, this is quite a niche usecase. This was also implemented for |
That depends, if |
No, the query is entirely independent from the data. We don’t know which data points are closest to the query without primals. |
In that case, my option of a dummy |
On second thought, I don't think you can systematically throw error on constructors even if |
@SouthEndMusic do you have an idea why the 1.6 tests fail and others don't? |
I spent a bit too much time writing tests for interpolants containing tracers in c993787 just to figure out we can't add overloads on them for the same reason @gdalle mentioned we can't overload the constructors:
The same applies here: we can easily overload
I afraid we'll have to leave it at supporting tracer inputs, at least for now. |
No description provided.