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

access to GAP's "tester" and "setter" functions #431

Merged
merged 2 commits into from
Jul 30, 2021
Merged

access to GAP's "tester" and "setter" functions #431

merged 2 commits into from
Jul 30, 2021

Conversation

ThomasBreuer
Copy link
Member

Use the macros @gapattribute and @gapwrap in order to get access to GAP's "tester" and "setter" functions for its attributes.
This had been suggested in issue #128.

The code requires the changes from the pull request oscar-system/GAP.jl/pull/647.
The branch has the same name as that of the current pull request.

(There are more places where the macros can be applied.
First we should agree on the setup, see below.)

  • The names of the tester/setter functions on the Julia side should perhaps be changed:
    Would has_derived_subgroup look better than hasderived_subgroup?
    (This is something to be changed in GAP.jl.)

  • Some of the functions in Oscar/src/Groups (conjugacy_classes, conjugacy_classes_subgroups, conjugacy_classes_maximal_subgroups) cannot be handled by the current @gapattribute, because they involve several calls to some function in GAP.Globals.
    The point is that the names of the tester and setter are "guessed" from the body of the given (getter) function.
    If there is just one call to a function in GAP.Globals then this is safe, otherwise we have to find some heuristic that is easy to document.

  • In a few cases (exponent, isfinite), I have added individual import statements because the current @gapattribute does not strip module names from the beginning of the function name; we could change the macro in order to avoid these import statements.

  • In order to keep the docstrings of the functions handled by @gapattribute, I did not see a better way than to move the code above the docstring.

  • Another point for the documentation is how to document the testers and setters. We could show three-line descriptions of the kind

    isperfect(G::Group)
    hasisperfect(G::Group)
    setisperfect(G::Group, val)
    

    in all relevant places, instead of just the first of these lines. Or would this become too long?

    (I have extended the export lists by the testers and setters. Is this o.k.?)

  • I have changed the behaviour of the functions only in one place. Up to now, trivial_subgroup constructed just a group on the Julia side, without calling GAP's GAP.Globals.TrivialSubgroup, but now the attribute on the GAP side is called, due to @gapattribute.
    The side-effect is that now the return value of trivial_subgroup has an empty list of generators, whereas this list contained the identity element before. This change seems to be no problem for the tests, but perhaps it has to be documented that the result of gens may be empty.

    The conceptual question is whether we really want to change trivial_subgroup such that the corresponding GAP function gets called.

    We can take the viewpoint that the Julia side should better avoid work on the GAP side where possible (one can create a trivial subgroup in Oscar without "help from GAP"), or we can argue that the GAP side should better be kept informed about knowledge that has been computed on the Julia side.

    Note that in the cases of boolean values, the stored GAP attributes really serve as caches; but already for a group order stored in GAP, a conversion may be necessary before the value can be used on the Julia side.

    One "border case" is ispgroup: Its return value is a tuple not just the boolean value that is stored on the GAP side. Any access from Oscar's ispgroup creates a new Julia object.

  • The following two inconsistencies should be dealt with in other pull requests:
    center and ischaracteristic are known in (and imported from) Hecke, but Oscar.Groups defines centre and ischaracteristic_subgroup.

@fingolfin
Copy link
Member

@ThomasBreuer I have closed and reopened this to see if it works, now that we use GAP.jl 0.6

But I think it will conflict with your other PR #583, so I am not sure how you want to proceed, even if the tests pass. But I would suggest that we should merge one of the two PRs as soon as possible.

Use the macros `@gapattribute` and `@gapwrap` in order to get access
to GAP's "tester" and "setter" functions for its attributes.
This had been suggested in issue #128.

The code requires the changes from the pull request oscar-system/GAP.jl/pull/647.
The branch has the same name as that of the current pull request.

(There are more places where the macros can be applied.
First we should agree on the setup, see below.)

- The names of the tester/setter functions on the Julia side should
  perhaps be changed:
  Would `has_derived_subgroup` look better than `hasderived_subgroup`?
  (This is something to be changed in GAP.jl.)

- Some of the functions in `Oscar/src/Groups` (`conjugacy_classes`,
  `conjugacy_classes_subgroups`, `conjugacy_classes_maximal_subgroups`)
  cannot be handled by the current `@gapattribute`,
  because they involve several calls to some function in `GAP.Globals`.
  The point is that the names of the tester and setter are "guessed" from
  the body of the given (getter) function.
  If there is just one call to a function in `GAP.Globals` then this is safe,
  otherwise we have to find some heuristic that is easy to document.

- In a few cases (`exponent`), I have added individual `import` statements
  because the current `@gapattribute` does not strip module names from the
  beginning of the function name; we could change the macro in order to
  avoid these `import` statements.

- In order to keep the docstrings of the functions handled by `@gapattribute`,
  I did not see a better way than to move the code above the docstring.

- Another point for the documentation is how to document the testers and
  setters.  We could show three-line descriptions of the kind
```
    isperfect(G::Group)
    hasisperfect(G::Group)
    setisperfect(G::Group, val)
```
  in all relevant places, instead of just the first of these lines.
  Or would this become too long?

  (I have extended the `export` lists by the testers and setters.
  Is this o.k.?)

- I have changed the behaviour of the functions only in one place.
  Up to now, `trivial_subgroup` constructed just a group on the Julia side,
  without calling GAP's `GAP.Globals.TrivialSubgroup`, but now the attribute
  on the GAP side is called, due to `@gapattribute`.
  The side-effect is that now the return value of `trivial_subgroup` has
  an empty list of generators,
  whereas this list contained the identity element before.
  This change seems to be no problem for the tests,
  but perhaps it has to be documented that the result of `gens` may be empty.

  The conceptual question is whether we really want to change `trivial_subgroup`
  such that the corresponding GAP function gets called.

  We can take the viewpoint that the Julia side should better avoid work on
  the GAP side where possible (one can create a trivial subgroup in Oscar
  without "help from GAP"),
  or we can argue that the GAP side should better be kept informed about
  knowledge that has been computed on the Julia side.

  Note that in the cases of boolean values,
  the stored GAP attributes really serve as caches;
  but already for a group order stored in GAP,
  a conversion may be necessary before the value can be used on the Julia side.

  One "border case" is `ispgroup`:
  Its return value is a tuple not just the boolean value that is stored on the
  GAP side.
  Any access from Oscar's `ispgroup` creates a new Julia object.

- The following two inconsistencies should be dealt with in other pull requests:
  `center` and `ischaracteristic` are known in (and imported from) Hecke,
  but Oscar.Groups defines `centre` and `ischaracteristic_subgroup`.
such that the documentation gets built without warnings

(I do not understand why Documenter.jl finds the docstring for
`nilpotency_class(G::GAPGroup)` but not the one for `fitting_subgroup(G::GAPGroup)`,
but this can be studied later.)
@ThomasBreuer
Copy link
Member Author

@fingolfin Thanks. I think it is easier to merge first this PR, and then to rebase #583.
I have adjusted the PR such that the documentation gets built without problems (locally).
I do not understand the warnings shown by Documenter.jl, but this problem can be inspected later.

@fingolfin fingolfin closed this Jul 30, 2021
@fingolfin fingolfin reopened this Jul 30, 2021
@ThomasBreuer
Copy link
Member Author

The test Documentation / build failed after my commit, and it fails again after closing/reopening the pull request.
The reason seems to be (in both cases) that output formats of the form Vector{Int64} are expected but evaluation yields Array{Int64,1}.

The messages about the failures say Error: doctest failure in ~/.julia/packages/AbstractAlgebra/KzYdE/src/....
This is irritating: Oscar's Project.toml wants AbstractAlgebra version "0.20.0", and this is stored at ~/.julia/packages/AbstractAlgebra/LJx6h in my local installation (where I can build the documentation without problems).
So why is the version from KzYdE used in the tests?
What am I missing?

@fingolfin fingolfin closed this Jul 30, 2021
@fingolfin fingolfin reopened this Jul 30, 2021
@fingolfin fingolfin merged commit 8b6cf6b into oscar-system:master Jul 30, 2021
@ThomasBreuer ThomasBreuer deleted the TB_macro_attributes branch July 30, 2021 14:48
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.

None yet

2 participants