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

libgap attribute access #26959

Closed
simon-king-jena opened this issue Dec 26, 2018 · 10 comments
Closed

libgap attribute access #26959

simon-king-jena opened this issue Dec 26, 2018 · 10 comments

Comments

@simon-king-jena
Copy link
Member

If foo is defined in libgap, then libgap.foo should access it. Currently it doesn't:

sage: libgap.fail
Traceback (most recent call last):
...
AttributeError: No such attribute: fail.
sage: libgap.eval('G := DihedralGroup(8)')
<pc group of size 8 with 3 generators>
sage: libgap.G
Traceback (most recent call last):
...
AttributeError: No such attribute: G.
sage: libgap.eval('G')
<pc group of size 8 with 3 generators>
sage: libgap.eval('fail')
fail

I guess libgap.fail would be more pythonic than libgap.eval('fail').

Note that only part of the above works in the pexpect interface:

sage: gap.eval('G := DihedralGroup(8);')
'<pc group of size 8 with 3 generators>'
sage: gap.G   # is some gap function, not what was defined above
G
sage: gap.fail # works as I would expect
fail

CC: @dimpase

Component: interfaces

Keywords: libgap attribute access

Issue created by migration from https://trac.sagemath.org/ticket/26959

@simon-king-jena
Copy link
Member Author

Changed keywords from none to libgap attribute access

@embray
Copy link
Contributor

embray commented Dec 28, 2018

comment:2

I agree. We had a long e-mail thread sort of discussing this, and it led to frustratingly few useful conclusions.

The problem is that right now there is a hard-coded list of what global variables in GAP can be accessed via attribute access on the libgap instance. I think this is partly intentional: The idea is that GAP has a ton of global variables (there's really just one big global namespace in GAP), so if you allow attribute access via libgap for any GAP global, it means libgap does not necessarily have a predictable API because it can depend on the version of GAP, what packages are loaded, etc.

I'm not totally sure this matters though. It should still be understood that the libgap object is a relatively low-level interface to GAP, and it does not necessarily have a "standard API" w.r.t. what GAP variables it provides access to. If nothing else, code using libgap that depends on functions (e.g. from GAP packages) that might not be defined should do a proper hasattr(libgap, "<funcname>") before attempting to use them, or at least be able to handle the resulting AttributeError if it's missing.

@embray embray self-assigned this Dec 28, 2018
@dimpase
Copy link
Member

dimpase commented Dec 28, 2018

comment:3

A further complication is that what's working in GAP depends upon the set of GAP packages installed and/or loaded.

@nbruin
Copy link
Contributor

nbruin commented Dec 28, 2018

comment:4

In the ECL wrapper I decided against this. If libgap.foo is just a shorthand for libgap.eval("foo"), then using libgap.foo is not going to be very efficient, because every time it will need to set up a wrapper object, which needs to hook into gap's garbage collector somewhere to ensure that the object doesn't get deleted before the wrapper disappears.
Much more efficient:

FOO=lib.eval("foo")
<do all kinds of things with FOO>

If you view these instructions like the "import" statements required in python, it won't seem so onerous.

It would be possible to create a namespace object that does have the above semantics, but caches the wrapper (it would basically be a defaultdict). This could then be used for permanent objects, such as functions (perhaps have some technology that also tries function_factory when appropriate?). I would say that the namespace of the libgap interface itself is not the best spot for this, though.

@simon-king-jena
Copy link
Member Author

comment:5

Replying to @nbruin:

In the ECL wrapper I decided against this. If libgap.foo is just a shorthand for libgap.eval("foo"), then using libgap.foo is not going to be very efficient,

Yes, that is what I was afraid of. And actually in the p_group_cohomology version that I am testing right now, I do what you suggest: I define Failure = libgap.eval('fail') in one module and import Failure from there.

So, I wouldn't mind to close this ticket as "won't fix".

@embray
Copy link
Contributor

embray commented Dec 31, 2018

comment:6

There's still something to be done here. See also the thread from https://groups.google.com/d/msg/sage-devel/iPTfFXUk8XU/UX3qr42xAQAJ

It's just not 100% clear to me, as a non-GAP user, what the best way forward is. I asked Alex Konovalov (specifically w.r.t. how to determine what functions in GAP are "standard") and he wasn't sure either, or didn't understand the question.

@embray
Copy link
Contributor

embray commented Dec 31, 2018

comment:7

Replying to @nbruin:

In the ECL wrapper I decided against this. If libgap.foo is just a shorthand for libgap.eval("foo"), then using libgap.foo is not going to be very efficient, because every time it will need to set up a wrapper object, which needs to hook into gap's garbage collector somewhere to ensure that the object doesn't get deleted before the wrapper disappears.

I don't follow you here. In the GAP interface there is no substantive difference between these cases: It still has to create a GapElement object to hold the returned object. You can see this yourself by looking at the sources in sage.libs.gap.libgap. There isn't really any substantive difference between libgap.eval("foo") and libgap.foo.

The only thing about the attribute getter is that it has some code for slightly fast-tracking functions, but even it is unnecessary (it effectively is just skipping checking the object's TNUM) and brittle (it assumes that everything in our hard-coded list of "common GAP functions" is still actually a function, when really GAP allows names to be redefined quite easily a la Python).

For functions that are used frequently there is also GAP.function_factory which is just the same as Gap.__getattr__ but it really really assumes that the name is bound to a function (when it might not be), and then it just caches the wrapper element. As you wrote, it might not be a bad idea if that caching happened automatically, especially for common functions that are not likely to be rebound. But it's not clear how to programatically determine what functions are "common functions that are not likely to be rebound".

@embray
Copy link
Contributor

embray commented Jan 15, 2019

comment:8

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

@embray embray modified the milestones: sage-8.6, sage-8.7 Jan 15, 2019
@embray
Copy link
Contributor

embray commented Mar 25, 2019

comment:9

Removing most of the rest of my open tickets out of the 8.7 milestone, which should be closed.

@embray embray removed this from the sage-8.7 milestone Mar 25, 2019
@embray embray added the pending label Mar 25, 2019
@embray
Copy link
Contributor

embray commented Feb 16, 2021

comment:10

This is fixed since a while ago by #27911. It is further improved on in #31404, since gappy does not use self.eval(attr) to do this, but simply uses the libgap API to check if GAP has a global named attr and raises an AttributeError if not, so this should alleviate nbruin's concerns mentioned here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants