Review request -- Mathieu fcns #60

brorson · 2025-09-06T01:59:01Z

This is a request to try the new Mathieu fcn impl. This new fcn impl has been discussed in #47 . I have updated the docstrings and other comment in this checkin, but the functionality has otherwise remained the same as that shown in the bug report.

I have run these fcns against the Scipy test suite using "spin test" and the test suite runs to completion. The Mathieu fcns fail, but that is expected since this is a new impl and the input arg is in radians, not degrees.

Please review and comment. My goal here is to request a review and find out what I need to change.

Stuart

lucascolley · 2025-09-06T11:44:33Z

I'm afraid I can't help with C++ expertise, but I wonder whether you could open a draft PR to SciPy which shows the tests running (albeit with failures)? No rush, just thought it might be worth it whilst you are waiting for review here and have some momentum. Let me know if you would like help with making the necessary changes to the SciPy submodule etc.

brorson · 2025-09-07T23:37:52Z

Thank you for your suggestion, @lucascolley. I will do that.

Meanwhile, I see the CI thing is running on my PR and I have build failures. I fixed one -- I was using the suffix 'd' when declaring double literals which turns out to be non-standard. I fixed that one and pushed to my forked repo. Not sure if CI runs continuously but I will continue fixing any bugs it finds.

Stuart

lucascolley · 2025-09-08T06:48:04Z

It is a GitHub limitation that since this is your first PR, CI runs require approval from someone with write access on the repo.

brorson · 2025-09-10T12:45:00Z

OK, I just submitted a PR on the main Scipy branch. The pull request includes:

My changes to the meson.build file which are required to successfully build the Mathieu stuff. The change is imply to add the Lapack dependencies to ufuncs and other build targets per @steppi 's suggestion.
I also mentioned the failing tests. The Mathieu tests now fail since the implementation is different and returns different results from the old impl. (The old tests didn't check correctness; they just checked that the output of the fcns didn't change.) The required action item here is to bypass the tests for the time being so the impls can be integrated into the codebase. I also offer to provide some Python-based tests written by my students which can be used in the future. Maybe it would make sense to have a Scipy branch we can work in and not bother the trunk?

I also submitted a new PR on the xsf repo to pick up a bugfix I implemented after looking at some of my last CI test failures: I had suffixed some double literals using 'd', like this: 2.0d. This is apparently not standard C++ so made some of the compilers barf. My mistake -- I fixed it and issued a new PR to you.

Sorry for the multiple PRs -- I'm new to the Scipy dev chain and so will fumble around a little bit as I try to get this change into the codebase.

Stuart

steppi · 2025-09-10T15:08:47Z

Thanks @brorson. I plan to spend some time in the next few weeks to improve testing, and we can add some test cases based on references from https://github.com/mathieuandspheroidalwavefunctions computed with quad precision. In the meantime, your main.c should be moved into the tests folder and turned into a proper Catch2 test file named something like xsf/tests/miscellaneous_tests/test_mathieu_funcs.cpp that will get picked up by the test harness. You don't have to use parquet tables, you can just hardcode the golden values into the test files. Let me know if you need any help with this. If you do this, I can set the tolerances in the relevant tests in xsf/tests/scipy_special_tests to infinity until reference values are recomputed.

rgommers · 2025-09-10T15:41:59Z

It is a GitHub limitation that since this is your first PR, CI runs require approval from someone with write access on the repo.

The usual workaround is to open a separate PR fixing a typo or other such trivial thing somewhere in the repo; once that's merged, CI will run here.

include/xsf/mathieu/LICENSE

brorson · 2025-09-10T22:47:53Z

Thank you, @steppi. Regarding the tests in main.c, sure I can move them to a file called "test_mathieu_fcns.cpp" and move them to xsf/tests/miscellaneous_tests/. Give me a day or two.

Once I do that I will check the mods into my GitHub repo and issue another PR so you know they're there.

brorson · 2025-09-10T23:57:26Z

@steppi, per your comments I moved the tests from main.c to a file xsf/tests/scipy_special_tests/test_mathieu.cpp in my local directory. But now I am looking at the other cpp files in that dir. It's clear that I need to do some homework and understand the formatting of the test files and how they are processed and then executed. Meanwhile, can I ask you to point me to some test files which are close to my use-case? My near-term goal is to just have a file which is picked up by the CI thing, runs some functions which test against golden values hard-coded into the file, then sets the right variables to tell the CI thing about passes/failures. Is there a particular file or files I can look at as an example?

Thanks,
Stuart

steppi · 2025-09-11T00:51:44Z

@steppi, per your comments I moved the tests from main.c to a file xsf/tests/scipy_special_tests/test_mathieu.cpp in my local directory. But now I am looking at the other cpp files in that dir. It's clear that I need to do some homework and understand the formatting of the test files and how they are processed and then executed. Meanwhile, can I ask you to point me to some test files which are close to my use-case? My near-term goal is to just have a file which is picked up by the CI thing, runs some functions which test against golden values hard-coded into the file, then sets the right variables to tell the CI thing about passes/failures. Is there a particular file or files I can look at as an example?

Thanks, Stuart

Thanks. I should have some time tomorrow to write a quick template for how the test file could be written and post it here

main.c to test_mathieu.cpp in the tests dir. I ran clang-format on all .h files.

brorson · 2025-09-11T01:07:52Z

Thank you.

jorenham

This looks pretty impressive!

I'm by no means an expert on the underlying maths or C++, but I left some (superficial) comments anyway; I hope you don't mind. I wasn't able to look at everything though, given the size and all.

One thing general pattern I've seen, is that there's quite a bit of code duplication. For reviewers that's not a lot of fun. So if you want to help speed up the review process, we'd appreciate it if you could try do something about the duplication. And there's no need to go wild with macro's; a couple of helper functions here and there could already help a lot.

I've also seen a several malloc and free calls. But since we're in C++ land, I don't think we need those. There's even the fancy std::mdspan you could use for vector- and matrix stuff, since we bundle a backport of it in kokkos/mdspan.hpp, which I imagine could be very useful here.

jorenham · 2025-09-19T20:42:56Z

include/xsf/mathieu.h

+ *
+ * Even parity characteristic values a.
+ *
+ * @param  m    Eigenvalue order.  Must be positive integer less than 500.


It should probably be an unsigned short or something then. I don't think that backwards compatibility is much of a concern here, but I might be wrong.

Judging by the if ((m < 0) || (m != floor(m))), it looks like m is allowed to be zero:

Suggested change

* @param m Eigenvalue order. Must be positive integer less than 500.

* @param m Eigenvalue order. Must be non-negative integer less than 500.

jorenham · 2025-09-19T20:45:11Z

include/xsf/mathieu.h

+template <typename T>
+T cem_cva(T m, T q) {


since it internally casts to- and from double, I don't think that there's any advantage to having it generic like this

jorenham · 2025-09-19T20:50:30Z

include/xsf/mathieu/besseljyd.h

+    //==================================================================
+    double besselj(int k, double z) {
+        // This is just a thin wrapper around the Bessel impl in the
+        // std library.


Suggested change

// std library.

// xsf library.

jorenham · 2025-09-19T20:54:17Z

include/xsf/mathieu/make_matrix.h

+ *
+ */
+
+#define SQRT2 1.414213562373095


There's a M_SQRT2 in config.h you could use instead

jorenham · 2025-09-19T20:57:07Z

include/xsf/mathieu/make_matrix.h

+    /*-----------------------------------------------
+    This creates the recurrence relation matrix for
+    the even-even Mathieu fcns (ce_2n).
+    Inputs:
+    N = matrix size (related to max order desired).
+    q = shape parameter.
+    Output:
+    A = recurrence matrix (must be calloc'ed in caller).
+    Return:
+    return code = 0 if OK.
+    -------------------------------------------------*/


nitpick: I think it would a bit easier to read if it would be a bit wider, so that there aren't as many linebreaks needed.

jorenham · 2025-09-19T21:54:12Z

include/xsf/mathieu/mathieu_coeffs.h

+        // Bail out if m is not even.
+        if (m % 2 != 0)
+            return -1;


this should probably fail loudly

jorenham · 2025-09-19T21:55:47Z

include/xsf/mathieu/mathieu_coeffs.h

+            return -1;
+
+        // Allocate recursion matrix
+        double *A = (double *)calloc(N * N, sizeof(double));


There's probably some smart C++ thingy that could be used that doesn't require the manual freeing. A std::vector perhaps.

jorenham · 2025-09-19T21:59:28Z

include/xsf/mathieu/mathieu_eigs.h

+            *a = std::numeric_limits<double>::quiet_NaN();
+            return SF_ERROR_DOMAIN;


it's usually better to fail loudly in case of invalid input

jorenham · 2025-09-19T22:01:42Z

include/xsf/mathieu/mathieu_fcns.h

+ * This is part of the Mathieu function suite -- a reimplementation
+ * of the Mathieu functions for Scipy.  This file holds the function
+ * implementations themselves.  The prototype was written in Matlab
+ * and validated.  This is a translation from Matlab to C.


Suggested change

* and validated. This is a translation from Matlab to C.

* and validated. This is a translation from Matlab to C++.

😉

jorenham · 2025-09-19T22:04:05Z

include/xsf/mathieu/mathieu_fcns.h

+namespace mathieu {
+
+    //==================================================================
+    int mathieu_ce(int m, double q, double v, double *ce, double *ced) {


xsf uses pass-by-reference in case of multiple scalar outputs

brorson · 2025-09-20T11:40:54Z

Thank you for the suggestions, @jorenham . I will go down the list and fix up my code based on your comments.

Regarding code duplication, yes I thought of using macros for the repeated stuff but since the repeated stuff is multi-line I figured any macro would be multi-line and hard to manage. However, I'll think about how to use macros again based on your comments, and may also see if I can stick some of the stuff into a separate inline fcn. I will want to comment heavily any macro or fcn to make clear what the calc is trying to do.

Regarding malloc() & free(), yeah, I am more of a C programmer than a C++ programmer. Does Scipy allow use of smart pointers? I see auto_ptr has been removed from C++ ... is there a preferred alternative?

(I will browse the codebase to see what is used, but it's always easier to ask ...)

Regarding failing loudly, do you mean throw an exception? I may be wrong, but since the xsf stuff supplies return codes I thought that was the preferred way to deal with errors. Please correct me if wrong.

Thx,
Stuart

brorson · 2025-09-20T11:54:51Z

Regarding code structure, mathieu.h living in the xsf directory provides the interface to the rest of Scipy. It is just a wrapper around my impls. My impls live in the mathieu subdirectory. Passing error codes within my stuff I have handled by just using integer flags, but I should fix that to conform to Scipy's error codes. Error codes passed back to Scipy from the interface fcns in mathieu.h are currently done using Scipy/xsf's predefined codes in error.h. I made a guess about what the different errors in the enum mean ... I hope I was close enough to be correct.

brorson · 2025-09-20T11:59:10Z

Regarding status of this project, I am currently bogged down with teaching and other responsibilities but hope to get back to this in a couple of weeks. Besides some clean-up per your suggestions (and my own list of things to clean up) I need to follow @rgommers suggestion about how to create a pull request which makes changes to two repos -- the main Scipy repo (where the Meson build config lives) and the xsf repo. I need to declare some dependencies in the build config stuff. That's waiting for me to do.

jorenham · 2025-09-23T11:11:10Z

Does Scipy allow use of smart pointers? I see auto_ptr has been removed from C++ ... is there a preferred alternative?

The cases I've seen could be replaced with a std::vector (docs) that you can pass around using .data() (docs)

I may be wrong, but since the xsf stuff supplies return codes I thought that was the preferred way to deal with errors

Anything user-facing that receives incorrect input returns NaN, generally speaking.
My reasoning here is that the major downside of return codes is that they're very easy to (accidentally) ignore, which is difficult for debugging. Exceptions must be explicitly dealt with, and if not, then you'll notice immediately, making it far less likely to miss things.
So since we're mostly talking about non-user-facing stuff here (unless I'm missing something), I'd choose exceptions over return codes, if I were in your shoes.
But either way, I meant this as a loose suggestion, not as a hard requirement or something.

functions to replace duplicated code.

brorson added 3 commits September 5, 2025 20:51

Update mathieu.h with improved docstrings and comments.

0adc5a6

Add new directory of Mathieu fcn impls.

9f62cf0

Make sure we return nan when error is encountered.

204e09d

Remove non-standard d in double constant literals.

b09c280

Merge branch 'scipy:main' into main

41c1f28

rgommers mentioned this pull request Sep 10, 2025

Add Lapack dependencies to meson.build for XSF scipy/scipy#23593

Draft

rgommers reviewed Sep 10, 2025

View reviewed changes

include/xsf/mathieu/LICENSE Outdated Show resolved Hide resolved

brorson added 2 commits September 10, 2025 19:28

Remove license file and place code under whatever license xsf uses.

93a41df

Merge branch 'main' of https://github.com/brorson/xsf_mathieu

b8911c1

I removed my Makefile since it's not used in the Scipy CI tool. I moved

5cbf45c

main.c to test_mathieu.cpp in the tests dir. I ran clang-format on all .h files.

Run clang-format on test_mathieu.cpp so it passes CI.

cdf598b

jorenham reviewed Sep 19, 2025

View reviewed changes

brorson added 4 commits September 24, 2025 18:59

Merge branch 'scipy:main' into main

cec5ca1

Replaced malloc/free with std::vector, updated some comments, added

9c850da

functions to replace duplicated code.

Merge branch 'main' of https://github.com/brorson/xsf_mathieu

72a5146

Merge branch 'scipy:main' into main

b25ba42

brorson added 2 commits September 26, 2025 20:02

Remove mathieu.h since it doesn't belong in this dir.

e32d63c

run clang-format on all .h files.

7614c4e

	* @param m Eigenvalue order. Must be positive integer less than 500.
	* @param m Eigenvalue order. Must be non-negative integer less than 500.

		*a = std::numeric_limits<double>::quiet_NaN();
		return SF_ERROR_DOMAIN;

	* and validated. This is a translation from Matlab to C.
	* and validated. This is a translation from Matlab to C++.

Uh oh!

Review request -- Mathieu fcns #60

Are you sure you want to change the base?

Review request -- Mathieu fcns #60

Uh oh!

Conversation

brorson commented Sep 6, 2025

Uh oh!

lucascolley commented Sep 6, 2025

Uh oh!

brorson commented Sep 7, 2025

Uh oh!

lucascolley commented Sep 8, 2025

Uh oh!

brorson commented Sep 10, 2025

Uh oh!

steppi commented Sep 10, 2025

Uh oh!

rgommers commented Sep 10, 2025

Uh oh!

Uh oh!

brorson commented Sep 10, 2025

Uh oh!

brorson commented Sep 10, 2025

Uh oh!

steppi commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brorson commented Sep 11, 2025

Uh oh!

jorenham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brorson commented Sep 20, 2025

Uh oh!

brorson commented Sep 20, 2025

Uh oh!

brorson commented Sep 20, 2025

Uh oh!

jorenham commented Sep 23, 2025

Uh oh!

Uh oh!

steppi commented Sep 11, 2025 •

edited

Loading