Skip to content

gap: switch to libgap API for GAP function calls #35887

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 20, 2023
Merged

gap: switch to libgap API for GAP function calls #35887

merged 1 commit into from
Jul 20, 2023

Conversation

fingolfin
Copy link
Contributor

@fingolfin fingolfin commented Jul 3, 2023 •
edited
Loading

📚 Description

Switch to the official libgap API for making function calls. This also simplifies the code quite a bit. And it makes it possible to call non-standard GAP functions (i.e. functions which are not T_FUNCTION but just an arbitrary objects for which a function call method has been installed).

As a caveat, I could not try this at all, as I currently cannot compile SageMath (it runs into weird errors, and I don't have time to debug this, sorry). In particular I just looked at the Cython documentation to find out how to get a raw pointer to the content of an array; but I am not even sure that I got the syntax right, let alone the semantics. But I figured it's worth a shot.

UPDATE: I decided to not try to use GAP_CallFuncArray anymore; it just is impossible for me to figure out the right way to call it from Cython when I can only test my changes by pushing them here. Instead a more modest but hopefully less problematic approach is now used.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@fchapoton
Copy link
Contributor

fchapoton commented Jul 7, 2023 •
edited
Loading

fails at

[sagelib-10.1.beta5]     Error compiling Cython file:
[sagelib-10.1.beta5]     ------------------------------------------------------------
[sagelib-10.1.beta5]     ...
[sagelib-10.1.beta5]                 sig_GAP_Enter()
[sagelib-10.1.beta5]                 sig_on()
[sagelib-10.1.beta5]                 if n == 0:
[sagelib-10.1.beta5]                     result = GAP_CallFunc0Args(self.value)
[sagelib-10.1.beta5]                 else:
[sagelib-10.1.beta5]                     result = GAP_CallFuncArray(self.value, n, a_raw.data.as_voidptr)
[sagelib-10.1.beta5]                                                                        ^
[sagelib-10.1.beta5]     ------------------------------------------------------------
[sagelib-10.1.beta5] 
[sagelib-10.1.beta5]     sage/libs/gap/element.pyx:2503:68: Cannot convert Python object to 'Obj *'

It seems to me that a_raw is a Python list.

@github-actions
Copy link

Documentation preview for this PR (built with commit 5af9711; changes) is ready! 🎉

@fingolfin fingolfin marked this pull request as ready for review July 10, 2023 08:54
@fingolfin
Copy link
Contributor Author

I gave up with my experiments, this is now much simpler but also hopefully will just work.

fchapoton
fchapoton approved these changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

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

ok, danke schön, ein mal wieder

@vbraun vbraun merged commit 0eaccb0 into sagemath:develop Jul 20, 2023
@fingolfin fingolfin deleted the mh/GAP_calls branch July 25, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fchapoton fchapoton fchapoton approved these changes

Assignees
No one assigned
Labels
c: packages: standard
Projects
None yet
Milestone
sage-10.1
Development

Successfully merging this pull request may close these issues.
5 participants
@fingolfin @fchapoton @vbraun @kwankyu @mkoeppe