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

Replace many uses of G.X by GapObj(X) #3671

Merged
merged 5 commits into from
May 2, 2024

Conversation

fingolfin
Copy link
Member

... so that ultimately we can refactor the underpinnings of GAPGroup etc. -- initially by dropping those slightly evil getproperty methods that produce G.X on demand.

In the future this enables other refactoring. E.g. we could contemplate adding a mechanism where one could replace GAPWrap.Foo(GapObj(x)) by GAPWrap.Foo(x) (namely: whenever the wrapper specifies that an argument must be a GapObj, as in e.g. GAP.@wrap ClassNames(x::GapObj)::GapObj -- then the generated wrapper may as well assume it has to convert anything else to GapObj.

Anyway: this PR does not strive to eliminate all accesses to .X -- first off, some of them are of course legit. Secondly, this is already very big and I'd rather resolve a large chunk of the issue now, before delaying this further and having to deal with merge conflicts...

Specifically, for
- MatrixGroup
- MatrixGroupElem
- SesquilinearForm
- GroupCoset
- GroupDoubleCoset
- GAPGroupConjClass

I left out SubgroupTransversal because there are additional
semantics on the Julia side invisible to the GAP object, so the
"conversion" is not always quite safe.

Also did nothing for GAPGroupClassFunction as there is no obvious
"free" conversion available there. We could still add a method,
but I'll leave that to a potential future PR.
Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Good.

Tests fail because GapObj is not known in the tests; the (few) tests that use GapObj already now call it as GAP.GapObj. Shall we better export GapObj, since now its use as a constructor in tests seems to be natural?

(It is interesting that GAPGroupConjClass has a field X whose value is the Oscar object given by acting_group, and that the underlying GAP object is stored in CC.)

@fingolfin
Copy link
Member Author

Tests fail because GapObj is not known in the tests; the (few) tests that use GapObj already now call it as GAP.GapObj. Shall we better export GapObj, since now its use as a constructor in tests seems to be natural?

Seems reasonably, so I've now exported GapObj.

(It is interesting that GAPGroupConjClass has a field X whose value is the Oscar object given by acting_group, and that the underlying GAP object is stored in CC.)

"Interesting" is putting it nicely -- I found it very confusing and surprising, and actually additional motivation from moving away from access to FOO.X to use of accessor functions with more self-explanatory names :-)

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

Successfully merging this pull request may close these issues.

2 participants