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: special: translate sph_bessel to C++, refactor cyl_bessel #20483

Merged
merged 13 commits into from
Apr 18, 2024

Conversation

izaid
Copy link
Contributor

@izaid izaid commented Apr 15, 2024

This PR translates and refactors various Bessel functions, both spherical and cylindrical. Currently a work in progress.

@github-actions github-actions bot added scipy.special C/C++ Items related to the internal C/C++ code base Cython Issues with the internal Cython code base Meson Items related to the introduction of Meson as the new build system for SciPy enhancement A new feature or improvement labels Apr 15, 2024
@lucascolley lucascolley removed the Meson Items related to the introduction of Meson as the new build system for SciPy label Apr 15, 2024
@lucascolley lucascolley changed the title ENH: Translate sph_bessel from Cython to C++, refactor cyl_bessel ENH: special: translate sph_bessel to C++, refactor cyl_bessel Apr 15, 2024
@izaid
Copy link
Contributor Author

izaid commented Apr 15, 2024

@steppi This is done, ready for review.

@izaid
Copy link
Contributor Author

izaid commented Apr 15, 2024

Right, I did a bit more here. Namely some other clean-up that needed to happen: standardising functions with multiple outputs to take references instead of pointers, remove sf_error.c so only sf_error.cc exists, etc. A lot of things are in better shape. Done here.

Copy link
Contributor

@steppi steppi left a comment

Choose a reason for hiding this comment

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

OK, there's a lot going on here @izaid . I'm going to give you a pass, because I know this is the last week you'll be able to devote to SciPy for a while, so I think it's fine to err on the side of getting as much as possible done. This was tough to review though due to a combination of many unrelated changes, and difficulty determining what was essential. The only change I've seen that is needed is ensuring Python.h is the first included header wherever it appears.

Let me know if the following list of changes is exhaustive.

  1. A large number of formatting changes.
  2. Passing references instead of pointers for ufuncs with multiple return values.
  3. _spherical_bessel.pxd translated into C++.
  4. Cylindrical bessel functions moved out of amos and into bessel.h.
    Can you give an account of what other changes, if any, were made besides moving
    these to a new file and changing some function names?
  5. sf_error.cc and sf_error.c combined to a single file, sf_error.c.
  6. New header scipy/special/special.h for functions where SciPy behavior deviate from behavior in C++ library (returning multiple values, where is from an independent function, runtime warning for noninteger parameters, returning NAN when result should be 0.
  7. New header for exponential integrals, translation of scaled_exp1 into C++ header.

Did I miss anything?

scipy/special/ufunc.h Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

@izaid and I talked about this offline, we both agree that having these files in the folder scipy/special makes the special in the filename redundant. We're just leaving it like this for now because there's already a generated _ufuncs.pyx which generates _ufuncs.c, and we want a way to distinguish between these until the generated ufuncs from _generate_pyx.py are moved to the new ufunc infrastructure.

#include "special/bessel.h"
#include "special/legendre.h"
#include "special/sph_harm.h"
#include "ufunc.h"

using namespace std;

using func_f_f1f1_t = void (*)(float, mdspan<float, dextents<ptrdiff_t, 1>, layout_stride>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, these pure formatting changes should be left out of big PRs like this. Keep it as is, it just slows down review because I have to check that nothing essential has changed here.

scipy/special/_gufuncs.cpp Outdated Show resolved Hide resolved
Comment on lines +36 to +37
double special_itstruve0(double x);
double special_it2struve0(double x);
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of anyone watching from the sidelines, the naming may seem confusing because special is temporarily overloaded right now in our naming scheme. When the C++ library of special functions gets a name, X, these files will become X_wrappers.cpp and X_wrappers.h. The function names here will be things like X_itstruve0 etc. Having things like that seems reasonable to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so the idea here is that the C wrappers are X_whatever for a C++ function X::whatever. Right now, X is "special", but we expect that to change in the future. This is pretty standard practice.

@izaid
Copy link
Contributor Author

izaid commented Apr 18, 2024

OK, there's a lot going on here @izaid . I'm going to give you a pass, because I know this is the last week you'll be able to devote to SciPy for a while, so I think it's fine to err on the side of getting as much as possible done. This was tough to review though due to a combination of many unrelated changes, and difficulty determining what was essential. The only change I've seen that is needed is ensuring Python.h is the first included header wherever it appears.

Let me know if the following list of changes is exhaustive.

1. A large number of formatting changes.

2. Passing references instead of pointers for ufuncs with multiple return values.

3. _spherical_bessel.pxd translated into C++.

4. Cylindrical bessel functions moved out of `amos` and into `bessel.h`.
   Can you give an account of what other changes, if any, were made besides moving
   these to a new file and changing some function names?

5. sf_error.cc and sf_error.c combined to a single file, sf_error.c.

6. New header `scipy/special/special.h` for functions where SciPy behavior deviate from behavior in C++ library (returning multiple values, where is from an independent function, runtime warning for noninteger parameters, returning `NAN` when result should be 0.

7. New header for exponential integrals, translation of scaled_exp1 into C++ header.

Did I miss anything?

OK, there's a lot going on here @izaid . I'm going to give you a pass, because I know this is the last week you'll be able to devote to SciPy for a while, so I think it's fine to err on the side of getting as much as possible done. This was tough to review though due to a combination of many unrelated changes, and difficulty determining what was essential. The only change I've seen that is needed is ensuring Python.h is the first included header wherever it appears.

Let me know if the following list of changes is exhaustive.

1. A large number of formatting changes.

2. Passing references instead of pointers for ufuncs with multiple return values.

3. _spherical_bessel.pxd translated into C++.

4. Cylindrical bessel functions moved out of `amos` and into `bessel.h`.
   Can you give an account of what other changes, if any, were made besides moving
   these to a new file and changing some function names?

5. sf_error.cc and sf_error.c combined to a single file, sf_error.c.

6. New header `scipy/special/special.h` for functions where SciPy behavior deviate from behavior in C++ library (returning multiple values, where is from an independent function, runtime warning for noninteger parameters, returning `NAN` when result should be 0.

7. New header for exponential integrals, translation of scaled_exp1 into C++ header.

Did I miss anything?

That's pretty comprehensive! A lot of refactoring and things to make life easier, plus translation of the spherical Bessel functions. I've made the changes requested now too.

scipy/special/ufunc.h Outdated Show resolved Hide resolved
Copy link
Contributor

@steppi steppi left a comment

Choose a reason for hiding this comment

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

I'm happy with this now.

@steppi steppi merged commit bfc1a81 into scipy:main Apr 18, 2024
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C/C++ Items related to the internal C/C++ code base Cython Issues with the internal Cython code base enhancement A new feature or improvement scipy.special
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants