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: Implemented methods 'extended' and '3/8-th' in simps function. fixes gh-4264 #4406

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aman-harness
Copy link

Implemented methods 'extended' and '3/8-th' in simps function. fixes gh-4264

method : {'composite', 'extended', 'second'}, optional
'composite' : Uses composite method of Simpson's rule.
'second' : Uses Simpson's 3/8-th rule for calculating integral.
'extended' : Uses extended Simpson's rule for calculating integral.
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be typeset as a ReST list, with leading dashes IIRC: "- 'composite': Uses composite method ...''

Copy link
Member

Choose a reason for hiding this comment

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

The methods need to be explained. Ideally, give exact formulas in the Notes section of the docstring.

@ev-br ev-br added scipy.integrate enhancement A new feature or improvement labels Jan 24, 2015
y = [0, 2.5, 5, 7.5, 10, 12.5, 15, 17.5, 20]
ans = simps(sample, x=y, method='extended')
assert_equal(ans, 88.6875)
assert_equal(val, 88.6875)
Copy link
Member

Choose a reason for hiding this comment

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

any tests for method='composite'?

Formula used in 'extended' method:-
Integral = dx/48*[17*y[0] + 59*y[1] + 43*y[2] + 49*y[3] + 48*X + 49*y[N-4] + 43*y[N-3]
+ 59*y[N-2] + 17*y[N-1]]
where X = (from i = 4 to i = N-5 ) Σ y[i]
Copy link
Author

Choose a reason for hiding this comment

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

@ev-br How to add summation formula here? Is this method fine?
Shouldn't I skip formula and just give the reference.

Copy link
Member

Choose a reason for hiding this comment

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

IMO formulas of this complexity are best written in TeX notation.

.. math::

  \frac{dx}{48} \left(  17 y_0 + 59 y_1 + 43 y_2 + 49 y_3 ... \right)

(Depending on how you write it, TeX can be anywhere between crystal clear and utterly unreadable).

In any case, unicode symbols are best avoided. Either use pseudo-python pseudocode, sum(y[i] for i in range(4, N-4)) or TeX: \sum_{i=4}{N-5} y_i

@aman-harness
Copy link
Author

@ev-br I have made all the changes. Please review it.

@aman-harness
Copy link
Author

@ev-br Just reminding. I have made all the suggested changes. Please review it.

@ev-br
Copy link
Member

ev-br commented Jul 22, 2017

Coming back to it after quite a while. TBH I'm not sure if it's worth it. These integration methods are valid, but I'm just not sure if they are used often enough (outside of teaching intro to numerical methods) to warrant adding them.

@aman-harness
Copy link
Author

@ev-br If it's already implemented I don't see any harm in adding the functionality to the function. If
someone was going to start on it, that would have been a different case. Anyways, you are the best person to decide if it should go in or not.

@tupui
Copy link
Member

tupui commented Mar 29, 2021

@ev-br should we close it or integrate this?

@ev-br
Copy link
Member

ev-br commented Mar 29, 2021

@tupui have to admit I don't remember anything about this PR (facepalm). And I won't be able to come back to it in a near future I'm afraid. So it's your call

@andyfaff
Copy link
Contributor

Besides equations there is no explanation of what the composite/extended/second options represent, and in what situations they should be used. From a usability view those would need to be explained in the notes section.

(I make this comment without knowledge of the advantages of this PR, nor whether it should be merged or not)

@tupui
Copy link
Member

tupui commented Mar 30, 2021

After looking back at Simpson and numerical integration in general, I think this might be useful to have the 3/8th Simpson rule as it becomes exact for n=3.

But the current interface is not optimal. In the end we could base everything on Newton-Cotes (if I am not missing anything). Also, it could be useful to return the error which can be easily calculated. Or it could be used to specify the tolerance.

@tupui
Copy link
Member

tupui commented Mar 30, 2021

The roadmap for the module is actually quite old... I feel like this could be reevaluated. But I am not an expert at all in numerical integration.

@lucascolley lucascolley added the needs-decision Items that need further discussion before they are merged or closed label Jan 5, 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 needs-decision Items that need further discussion before they are merged or closed scipy.integrate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature or bug? : scipy.integrate.simps : result for integration of pulse is position dependent
5 participants