Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial support for calling GNU Scientific Library (GSL) special functions from Loopy’s C target infrastructure (currently wired into the GNU-libc-enabled C target).
Changes:
- Introduces
GSLCallableto map Loopy callables togsl_sf_*functions and emit required C preambles. - Adds
get_gsl_callables()(currently exposinghyperg_2F1) and registers these callables inCWithGNULibcASTBuilder.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| callables.update(get_gnu_libc_callables()) | ||
| callables.update(get_gsl_callables()) |
There was a problem hiding this comment.
Registering GSL callables for CWithGNULibcASTBuilder makes ExecutableCWithGNULibcTarget kernels that use gsl_sf_* likely fail at build/link time unless the CCompiler is configured with the correct include_dirs and libraries (e.g. gsl, often also gslcblas). Consider providing a dedicated target/compiler preset for GSL (or updating this target) that wires in the required toolchain options, or otherwise surfacing a clear error when GSL headers/libs aren't available.
There was a problem hiding this comment.
This is a fair point. Here I'm abusing of the GNULibc classes, for which we should only register functions that depend on GNULibc. Should I create separate GSL counterparts?
There was a problem hiding this comment.
I'm not sure I love either solution. I'm not sure I want a thicket of derived classes, but I agree that adding this by default here isn't awesome.
It might be best to leave it to the user to have their own derived class. Of course, this would be easier if known_callables lived in TargetBase, not the ASTBuilder. Let me see if that's a refactor that AI can do: #988.
| def get_gsl_callables(): | ||
| # Support special functions from | ||
| # https://www.gnu.org/software/gsl/doc/html/specfunc.html | ||
| func_ids = ["hyperg_2F1"] | ||
| return {id_: GSLCallable(id_) for id_ in func_ids} |
There was a problem hiding this comment.
New callable support (hyperg_2F1 via GSL) is introduced here, but there is no corresponding test (compare the existing test_glibc_bessel_functions coverage). Adding a test that compiles/runs an ExecutableCWithGNULibcTarget kernel calling hyperg_2F1 and validates against a reference (e.g. scipy.special.hyp2f1/mpmath), with a skip if GSL isn't available, would help prevent regressions.
| arg_num_to_dtype = {cast("int", id): t for id, t in arg_id_to_dtype.items()} | ||
| arg_num_to_dtype[-1] = arg_num_to_dtype[max(arg_num_to_dtype)] | ||
|
|
||
| name_in_target = f"gsl_sf_{name}" | ||
| return (self.copy(name_in_target=name_in_target, | ||
| arg_id_to_dtype=constantdict(arg_num_to_dtype)), | ||
| clbl_inf_ctx) |
There was a problem hiding this comment.
with_types does not validate the expected signature for GSL special functions (e.g. hyperg_2F1 is a fixed-arity function) and currently sets the return dtype to the dtype of the last argument via max(arg_num_to_dtype). This can mis-specialize the callable (wrong return type, accepting wrong number/types of args) and can also raise a ValueError on max() for invalid/empty calls. Consider explicitly enforcing the correct arity and specializing both argument and return dtypes to the actual GSL signature (typically double), relying on Loopy's casting to convert inputs when needed.
| arg_id_to_dtype=constantdict(arg_num_to_dtype)), | ||
| clbl_inf_ctx) | ||
|
|
||
| def generate_preambles(self, target): |
There was a problem hiding this comment.
Could you add the missing annotations (here and below)? (bpr will also complain I think.)
There was a problem hiding this comment.
Would it make sense to test this? (Not saying we should, just raising the possibility.)
| for id in arg_id_to_dtype: | ||
| if not isinstance(id, int): | ||
| raise LoopyError(f"'{name}' can take only positional arguments") |
There was a problem hiding this comment.
If you do this as a list comprehension, you may not have to cast below.
| arg_num_to_dtype = {cast("int", id): t for id, t in arg_id_to_dtype.items()} | ||
| arg_num_to_dtype[-1] = arg_num_to_dtype[max(arg_num_to_dtype)] | ||
|
|
||
| name_in_target = f"gsl_sf_{name}" | ||
| return (self.copy(name_in_target=name_in_target, | ||
| arg_id_to_dtype=constantdict(arg_num_to_dtype)), | ||
| clbl_inf_ctx) |
There was a problem hiding this comment.
There's probably some argument type validation missing here. I'm willing to believe that this uses overloads for types (C11-style), but that's probably still a finite set?
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| return {id_: GNULibcCallable(id_) for id_ in func_ids} | ||
|
|
||
|
|
||
| def get_gsl_callables(): |
There was a problem hiding this comment.
Add this to the documentation somewhere.
| callables.update(get_gnu_libc_callables()) | ||
| callables.update(get_gsl_callables()) |
There was a problem hiding this comment.
I'm not sure I love either solution. I'm not sure I want a thicket of derived classes, but I agree that adding this by default here isn't awesome.
It might be best to leave it to the user to have their own derived class. Of course, this would be easier if known_callables lived in TargetBase, not the ASTBuilder. Let me see if that's a refactor that AI can do: #988.
No description provided.