-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
ENH: Add polynomial features and extra basis functions to scipy.interpolate.rbf #7157
Conversation
34b44f2
to
482cd5e
Compare
This now needs a rebase, as it crossed with gh-7161. You can either do a rebase yourself, or grab the rebased branch: https://github.com/ev-br/scipy/commits/pr/7157 |
scipy/interpolate/rbf.py
Outdated
self.A = self._init_function(r) - np.eye(self.N)*self.smooth | ||
self.nodes = linalg.solve(self.A, self.di) | ||
self.A_aug = self._generate_augmented_matrix(self.xi.T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be stored on the instance: the matrix is only used once and can be large. If really needs be, it can be made into a property, or just discarded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
scipy/interpolate/rbf.py
Outdated
combs = combinations(n_dims, deg) | ||
for i, c in enumerate(combs): | ||
POLY[:, i] = xi[:, c].prod(axis=1) | ||
return bmat([[self.A, POLY], [POLY.T, None]], format='csr') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nitpick: This function is a bit confusing: it receives xi
as an argument, and is being called with self.xi
; it also uses self.degree
which it reads off the instance; it attaches self.n_features
but returns self.A_aug
. This could probably be combined into def _compute_nodes(self):
which computes self.nodes
and discards all local variables, e.g. A_aug
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed all the above.
scipy/interpolate/rbf.py
Outdated
n_points, n_dims = xi.shape | ||
# closed-form formula for number of polynomial coefficients | ||
deg = self.degree if self.degree else -1 | ||
self.n_features = int(comb(deg + n_dims, n_dims)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are features
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
features
are the generated polynomial coefficients. Updated the variable name and description.
It would be helpful to explain a bit what is going on here. Also references should be added to the docstring. It would also be helpful to explain a connection to the suggestions in |
482cd5e
to
a8a8488
Compare
Oops, something went wrong on the git side. Don't merge master, please rebase on master instead, yhen force-push (the latter is key). Do you need help untangling this? |
Yeah @ev-br trying to fix this now, but how do I rebase all the previous commits and keep my latest ones? |
3b0eef8
to
dbb346c
Compare
You squashed your work into this single commit, dbb346c
This will replace the the current contents of this PR by that single commit. |
dbb346c
to
c64d285
Compare
Done, thanks! Just couldn't figure out the new-branch:rbf-polynomials part. |
c291427
to
5b9252f
Compare
|
||
If callable, then it must take 2 arguments (self, r). The epsilon | ||
parameter will be available as self.epsilon. Other keyword | ||
arguments passed in will be available as well. | ||
|
||
degree : integer, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description needs to be clarified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also "adjustable constant" reads a bit oddly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified.
scipy/interpolate/rbf.py
Outdated
epsilon : float, optional | ||
Adjustable constant for gaussian or multiquadrics functions | ||
- defaults to approximate average distance between nodes (which is | ||
a good start). | ||
m : float, optional | ||
Adjustable constant for polyharmonic splines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, what is m, how do you adjust it? Guidelines here would be helpful to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see how this works now. Perhaps something like "A parameter used to construct polyharmonic splines. Large m yields ... Typical values lie between ... and ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. I specified that m refers to the degree of the polyharmonic spline.
scipy/interpolate/rbf.py
Outdated
m : float, optional | ||
Adjustable constant for polyharmonic splines. | ||
p : float, optional | ||
Adjustable constant for bessel functions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
scipy/interpolate/rbf.py
Outdated
def _compute_nodes(self): | ||
""" | ||
First, we generate all possible combinations of the input over all | ||
dimensions for the polynomial features. Then, these are stacked to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"polynomial features"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to powers of input dimensions.
scipy/interpolate/rbf.py
Outdated
|C.T| 0 | | ||
--------- | ||
|
||
where C is a matrix of size (n, num_coeffs) generated from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More detail is needed to understand what happens here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
5b9252f
to
f5fb436
Compare
@treverhines I just came across your RBF work at https://github.com/treverhines/RBF I was wondering if you could take a look at this pull-request, and consider whether we could work together to include your existing work into SciPy? Thanks! |
@evanlimanto very sorry for this PR slipping under the radar. I don't think we will be merging this in the end, now that the RBFInterpolator is in. Your work is much appreciated regardless! What is the status here, is there something from this PR to be ported to the RBFInterpolator interface? @treverhines @segasai @stefanv @evanlimanto |
It would be good to ask @treverhines that question. (Woops, see you already did :) ). |
Ok, let's close this. If there's something to port to the current RBF framework, we can always reopen or cherry-pick. Thank you very much @evanlimanto , your work is much appreciated regardless! |
See section 2.1 of this paper.