Skip to content
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: improve integrate.simpson for even number of points #18209

Merged
merged 10 commits into from
Apr 25, 2023

Conversation

andyfaff
Copy link
Contributor

@andyfaff andyfaff commented Mar 29, 2023

Closes #5618

This PR improves the accuracy of integrate.simpson when there is an even number of points (odd number of intervals).

Historical behaviour has been to use trapezoidal integration for the last/first intervals and adding it on to the _basic_simpson sum of the other intervals. This is what the even kwd was used for. When even='avg' the mean of last and first approaches was returned.

It was pointed out in #18151 that there is a more accurate way of dealing with this situation, as outlined on wikipedia. This uses a parabolic segment over the last three points to calculate the sum of the last interval, thereby using simpson behaviour for the last interval as well.

This PR:

  • adds the machinery for performing the improved calculation for the last interval when there is an even number of samples.
  • this machinery is accessed by adding a 'simpson' option to the even kwd.
  • Since even='simpson' should offer improved behaviour in most cases (i.e. it continues the quadratic nature of the rest of the summation) this now becomes the default for the even kwd.
  • Because even='simpson' offers this improved behaviour it shouldn't be necessary to access the {'first', 'last', 'avg'} options anymore. It's therefore suggested that the even kwd be deprecated and removed in a future version of scipy. After that point the 'simpson' behaviour will be used all the time.
  • To achieve this deprecation the even kwdis removed from the function signature and replaced by**kwds. Existing code that uses even` will continue to work, but will receive a warning.
  • If the integration axis only has two points (i.e. impossible to form a parabolic segment with the last three points) then the integration is done trapezoidally.
  • Tests are added to check simpson integration over multiple axes, for odd and even number of points in the integration axis. These tests were missing and strengthen the test suite.

If there is downstream code using simpson they will:

  • start getting the DeprecationWarning. This may come as a surprise as the even keyword for simpson has been present for a long time.
  • get different results if they integrate with an even number of points. The new behaviour is more accurate, but people may complain that their code isn't giving the same numbers any more.

@andyfaff
Copy link
Contributor Author

The macOS-meson workflow has started to fail, not due to changes introduced in this PR.

@j-bowhay j-bowhay added enhancement A new feature or improvement scipy.integrate deprecated Items related to behavior that has been deprecated labels Mar 29, 2023
Copy link
Member

@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

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

Thanks, some comments around on the deprecation. I think there is a backwards compatibility issue for anyone using positional arguments (see my comment below)

scipy/integrate/_quadrature.py Outdated Show resolved Hide resolved
scipy/integrate/_quadrature.py Outdated Show resolved Hide resolved
scipy/integrate/_quadrature.py Outdated Show resolved Hide resolved
scipy/integrate/_quadrature.py Outdated Show resolved Hide resolved
scipy/integrate/_quadrature.py Outdated Show resolved Hide resolved
andyfaff and others added 2 commits March 29, 2023 21:38
Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
@tupui tupui changed the title ENH: improve integrate.simpson for even number of points. Closes #5618 ENH: improve integrate.simpson for even number of points Mar 29, 2023
@tupui tupui added this to the 1.11.0 milestone Mar 29, 2023
Copy link
Member

@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

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

Thanks @andyfaff all the maths looks in order to me and this gives a nice improvement

@j-bowhay
Copy link
Member

I cannot remember, was there a post on the mailing list about the deprecation aspect?

@andyfaff
Copy link
Contributor Author

andyfaff commented Apr 19, 2023

Have just written an email. I'd give it a couple of days before merging. Do you have any concerns about implementation?

@andyfaff andyfaff closed this Apr 19, 2023
@andyfaff andyfaff reopened this Apr 19, 2023
@andyfaff
Copy link
Contributor Author

@j-bowhay

@j-bowhay
Copy link
Member

Thanks had another skim and tried the example from the original issue on this branch. This gives a clear improvement so let get it in

@j-bowhay j-bowhay merged commit 572a373 into scipy:main Apr 25, 2023
25 checks passed
@j-bowhay j-bowhay mentioned this pull request Apr 25, 2023
29 tasks
@andyfaff andyfaff deleted the simpson branch April 28, 2023 11:09
vnmabus added a commit to GAA-UAM/scikit-fda that referenced this pull request Mar 6, 2024
SciPy version 1.11 changes how Simpson quadrature is computed with an
even number of points (scipy/scipy#18209).

This caused some test to fail, as the values of the integrals and
metrics used change, causing in some cases several discrepancies
(#551).

This has been fixed to make the tests compatible with both previous
versions and versions after 1.11, in two main ways:
- By making some comparisons more lenient, allowing for higher relative
or absolute tolerances.
- By rewriting some tests in order to be less sensitive to changes in
quadrature by design.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated Items related to behavior that has been deprecated enhancement A new feature or improvement scipy.integrate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Solution for low accuracy of simps with even number of points
3 participants