Skip to content

feat: Added numerical integration of generic functions #201

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

Merged
merged 5 commits into from
Jul 14, 2023

Conversation

s-kuberski
Copy link
Collaborator

I have added a routine that allows to numerically integrate a one-dimensional function using scipy.integrate.quad. The parameters of the function, as well as the integration bounds, may be Obs.

@fjosw: I was not completely sure about the magic that is applied when constructing the derived_observable maybe you can check it, the current version seems to do the correct thing.

I think the tests should check the relevant cases as one can very nicely compare the numeric with the analytic integration. I hope that I did not overlook any edge case.

@s-kuberski s-kuberski requested a review from fjosw as a code owner July 14, 2023 08:55
@fjosw
Copy link
Owner

fjosw commented Jul 14, 2023

Looks good! I will do some explicit testing later.

Concerning the interface I would maybe propose to have a separate submodule integrate and then to rename the new function to quad to mimic the scipy interface. This would also make room for potential future implementations of other integration methods.

@s-kuberski
Copy link
Collaborator Author

Thanks for looking into it. I refactored the code and added the new module.

Copy link
Owner

@fjosw fjosw left a comment

Choose a reason for hiding this comment

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

I played around a bit with the new function and everything looks good.

I made a small correction to the docstring and added two more trivial test cases.

@s-kuberski
Copy link
Collaborator Author

There is one case, where this routine fails, namely when no Obs is involved, at all. In this case, derived_observable throws an exception.
One could circumvent this by adding

if len(derivint) == 0:
    return integration_result

before constructing the derived observable, but maybe you have something smarter in mind?

@fjosw
Copy link
Owner

fjosw commented Jul 14, 2023

There is one case, where this routine fails, namely when no Obs is involved, at all. In this case, derived_observable throws an exception. One could circumvent this by adding

if len(derivint) == 0:
    return integration_result

before constructing the derived observable, but maybe you have something smarter in mind?

That sounds like a good solution, do you want to quickly push it?

@fjosw fjosw merged commit 6dcd0c3 into fjosw:develop Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants