Skip to content

Conversation

j-bowhay
Copy link
Member

Reference issue

https://discuss.scientific-python.org/t/inclusion-of-aaa-rational-approximation-in-scipy/1282

What does this implement/fix?

This PR implements the core AAA algorithm for rational approximation. For more background please see the mailing list post linked above. There are a number of extra features that would be good to include however I would prefer to leave these as follow ups to keep the diff manageable.

Features:

  • Computes near-optimal rational approximation for provided data which can then be evaluated
  • Can compute the roots, poles and residues of the rational approximation

Planned Features:

  • Cleanup of spurious poles
  • Differentiation of approximation
  • Lawson iteration for optimal approximation

Additional information

cc @izaid @nakatsukasayuji

@j-bowhay j-bowhay requested a review from ev-br as a code owner July 12, 2024 10:55
@github-actions github-actions bot added scipy.interpolate Meson Items related to the introduction of Meson as the new build system for SciPy enhancement A new feature or improvement labels Jul 12, 2024
Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to provide some comparison with pade? And would any kind of addition to benchmarks be appropriate?

@lucascolley lucascolley removed the Meson Items related to the introduction of Meson as the new build system for SciPy label Jul 12, 2024
@j-bowhay j-bowhay added this to the 1.15.0 milestone Jul 12, 2024
@j-bowhay
Copy link
Member Author

Would it be possible to provide some comparison with pade?

Yes I could add a sentence in the notes

And would any kind of addition to benchmarks be appropriate?

Done

@lucascolley
Copy link
Member

Yes I could add a sentence in the notes

Sorry, I meant adding some results to this PR conversation to justify why one may want to use AAA instead of pade.

@j-bowhay
Copy link
Member Author

Yes I could add a sentence in the notes

Sorry, I meant adding some results to this PR conversation to justify why one may want to use AAA instead of pade.

Sure, whilst both are rational approximations, they are fundamentally quite different. Pade is concerned with rational approximation of a Taylor series at a point where as AAA approximates a function on a set of points. Furthermore, Pade approximation is a quotient of polynomials which breaks down when the poles are clustered whereas AAA use a barycentric representation which turns out to be incredibly stable

@j-bowhay
Copy link
Member Author

j-bowhay commented Jul 13, 2024

CI is green so this is now ready for review

Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks clean to me!

Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial cursory review: only looked at the docs and the overall structure.
Small minor comments are mostly optional. Additionally,

  • @cached_property: are we sure it plays ball with no-gil? Overall I'd suggest keeping properties for basically fetching things off the object and computations can as well be methods.
  • Consider making the input validation stricter. array or callable, auto raveling the z arrays are best avoided IMO.
  • The amount of with np.errstate("ignore") is a bit worrying. I did not look into the math though, maybe there's no way around

@j-bowhay j-bowhay requested review from lucascolley and ev-br July 22, 2024 11:40
Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once these points are addressed, thanks Jake!

@lucascolley lucascolley added the needs-release-note a maintainer should add a release note written by a reviewer/author to the wiki label Jul 22, 2024
j-bowhay and others added 2 commits July 22, 2024 14:13
[skip ci]

Co-authored-by: Lucas Colley <lucas.colley8@gmail.com>
@j-bowhay
Copy link
Member Author

@ev-br I wanted to check whether there was anything outstanding from your initial review or any additional comments?

I have removed the cached properties and removed the with np.errstate("ignore") in a few places where they weren't necessary. Where they are necessary I have added comments explaining why.

I would be happy to look at any upcoming interpolate prs in return!

@ev-br
Copy link
Member

ev-br commented Jul 28, 2024

Give me a couple of days @j-bowhay to take a second look? Unless you need it in earlier, in which case I'm happy to declare it good enough :-).

@j-bowhay
Copy link
Member Author

Give me a couple of days @j-bowhay to take a second look? Unless you need it in earlier, in which case I'm happy to declare it good enough :-).

That would be great, thank you!

Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good modulo a few minor points of confusion over the docs and a possible FUD about licenses. Two more bigger questions, one about the math, and one about the design.

  • the AAA paper talks about some way of dealing with numerical doublets--- IIUC, if run for long enough, the algorithm will produce poles with residues of the order of machine epsilon. Do we want/need to add some step to deal with those? Or, if not, add some documentation on how to deal with these on the user side?
  • the PR adds a class which does two different things: constructs a rational function with a certain recipe, and adds a callable object which represents a rational function. Have you considered splitting the latter into a separate entity (a BarycentricInterpolator? No, this name is taken by something related but different and not very perf-friendly). I personally like a pattern of having a factory function to return an instance of a class which does evaluations, knows about poles, residues and zeros etc. I'm not going to insist on exactly this, just would encourage you to give it a thought, also depending on what the future plans are. My main concern is to not paint outselves into a corner API-wise.

None of this is blocking though, and can be left for a possible follow-up. So consider this PR as approved from my side.

from `values`, and :math:`w_1,\dots,w_m` are real or complex weights. The algorithm
then proceeds to select the next support point :math:`z_{m+1}` from the remaining
unselected `points` such that the nonlinear residual
:math:`|f(z_{m+1}) - n(z_{m+1})/d(z_{m+1})|` is maximised. The algorithm terminates
Copy link
Member

@ev-br ev-br Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

residual ... is minimized? Ouch, no. It selects as the m+1) -th point the one, k, from previously unselected points, which corresponds to the maximal value of the residual, is this correct?
What confused me was that I was expecting an LSQ type process under which some residual is minimized. I guess this is what you're saying, actually. Consider rephrasing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to make the two parts of the algorithm clearer: the next support point is chosen to maximise the residual but the weights are chosen to minimise the residual

unselected `points` such that the nonlinear residual
:math:`|f(z_{m+1}) - n(z_{m+1})/d(z_{m+1})|` is maximised. The algorithm terminates
when this maximum is less than ``rtol * np.linalg.norm(f, ord=np.inf)``. This means
the interpolation property is only satisfied up to a tolerance. The weights are
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is satisfied exactly on the subset of the data points, which were selected? Feel free to discount my confusion if you think it's just me, or rephrase.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, will add an extra little bit

\text{minimise}|fd - n| \quad \text{subject to} \quad
\sum_{i=1}^{m+1} w_i = 1,

over the unselected elements of `points`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to add that the minimization is over the weights w_j

@j-bowhay
Copy link
Member Author

j-bowhay commented Aug 2, 2024

Looks good modulo a few minor points of confusion over the docs and a possible FUD about licenses. Two more bigger questions, one about the math, and one about the design.

Thanks for the review!

  • the AAA paper talks about some way of dealing with numerical doublets--- IIUC, if run for long enough, the algorithm will produce poles with residues of the order of machine epsilon. Do we want/need to add some step to deal with those? Or, if not, add some documentation on how to deal with these on the user side?

Yes good spot, I have code written which implements this. I had planned to leave this as a follow up to keep the diff smaller but could add it here if you prefer

  • the PR adds a class which does two different things: constructs a rational function with a certain recipe, and adds a callable object which represents a rational function. Have you considered splitting the latter into a separate entity (a BarycentricInterpolator? No, this name is taken by something related but different and not very perf-friendly). I personally like a pattern of having a factory function to return an instance of a class which does evaluations, knows about poles, residues and zeros etc. I'm not going to insist on exactly this, just would encourage you to give it a thought, also depending on what the future plans are. My main concern is to not paint outselves into a corner API-wise.

Yes good point I have thought about this as we may want to add other weights in the future such as Floater-Hormann. In which case my plan was to have a base class _BarcentricRational which implements the __call__, roots, poles, residues methods exactly as they are currently implemented and then AAA and FloaterHormann would just implement a _compute_weights method which is called in the constructor. I think there would be nothing stopping this refactor in the future if we wanted to. Does this sound alright to you?

Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great Jake. Feel free to hit the green button with or without my other minor points of confusion. Am looking forward to the follow-ups :-).

@j-bowhay
Copy link
Member Author

j-bowhay commented Aug 3, 2024

I have hopefully cleared up some points of confusion in the docs @ev-br but if not happy to polish in follow up prs!

@j-bowhay j-bowhay merged commit a11488c into scipy:main Aug 3, 2024
39 checks passed
@j-bowhay
Copy link
Member Author

j-bowhay commented Aug 3, 2024

Thanks @ev-br @mdhaber @lucascolley @stefanv @rkern @izaid @nakatsukasayuji for your help!

@lucascolley
Copy link
Member

congrats Jake, great work! Happy to review adding array API standard support, even if partially blocked on linalg for now.

And congrats on the inclusion in SciPy @nakatsukasayuji !

@mdhaber mdhaber removed the needs-release-note a maintainer should add a release note written by a reviewer/author to the wiki label Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement scipy.interpolate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants