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

Document RootFFE? Merge it with NthRoot? What about Root? #2131

Closed
fingolfin opened this issue Jan 30, 2018 · 6 comments · Fixed by #4471
Closed

Document RootFFE? Merge it with NthRoot? What about Root? #2131

fingolfin opened this issue Jan 30, 2018 · 6 comments · Fixed by #4471
Labels
kind: discussion discussions, questions, requests for comments, and so on topic: documentation Issues and PRs related to documentation

Comments

@fingolfin
Copy link
Member

@hulpke added a new operation LowIndexSubgroups in PR #683, but it is currently undocumented (well, there is a documentation comment, but it is not hooked up into the the reference manual).

I wonder if this is intentional, or whether perhaps LowIndexSubgroups should be documented (and then also on the stable-4.9 branch)?

@fingolfin
Copy link
Member Author

I actually wonder the same about RootFFE, which was already added in gap-4.8 (and later revised in GAP 4.9)

@fingolfin fingolfin changed the title Document LowIndexSubgroups ? Document LowIndexSubgroups ? RootFFE? Jan 30, 2018
@olexandr-konovalov
Copy link
Member

Just to remind about #855 and #1796 discussing how to find documentation which is not linked into Manuals

@fingolfin
Copy link
Member Author

@alex-konovalov I mention these two function specifically because RootFFE is mentioned in the draft of the GAP 4.9 release notes -- but if it stays undocumented, it should be removed there! For else we'd be suggesting to users that RootFFE is safe to use in their own code, when it isn't. (That said, I'd greatly prefer for it to become documented!)

Somewhat similar for LowIndexSubgroups: I wanted to improve the description for PR #683 in the release notes, and wanted to mention LowIndexSubgroups in there, but then realized it is undocumented...

hulpke added a commit to hulpke/gap that referenced this issue Feb 1, 2018
@hulpke hulpke removed their assignment Feb 1, 2018
hulpke added a commit to hulpke/gap that referenced this issue Feb 1, 2018
as requested in gap-system#2131

Also added `LowLayerSubgroups` and minor rephrasing.

Also redid part of gap-system#683, as for some reason (probably my stupidity)
part of gap-system#683 had fallen out of master. Added it again.
@olexandr-konovalov olexandr-konovalov added the topic: documentation Issues and PRs related to documentation label Feb 3, 2018
fingolfin pushed a commit to fingolfin/gap that referenced this issue Feb 5, 2018
as requested in gap-system#2131

Also added `LowLayerSubgroups` and minor rephrasing.

Also redid part of gap-system#683, as for some reason (probably my stupidity)
part of gap-system#683 had fallen out of master. Added it again.
fingolfin pushed a commit to fingolfin/gap that referenced this issue Feb 7, 2018
as requested in gap-system#2131

Also added `LowLayerSubgroups` and minor rephrasing.

Also redid part of gap-system#683, as for some reason (probably my stupidity)
part of gap-system#683 had fallen out of master. Added it again.
fingolfin pushed a commit to fingolfin/gap that referenced this issue Feb 8, 2018
as requested in gap-system#2131

Also added `LowLayerSubgroups` and minor rephrasing.

Also redid part of gap-system#683, as for some reason (probably my stupidity)
part of gap-system#683 had fallen out of master. Added it again.
fingolfin pushed a commit that referenced this issue Feb 9, 2018
as requested in #2131

Also added `LowLayerSubgroups` and minor rephrasing.

Also redid part of #683, as for some reason (probably my stupidity)
part of #683 had fallen out of master. Added it again.
fingolfin pushed a commit to fingolfin/gap that referenced this issue Mar 7, 2018
as requested in gap-system#2131

Also added `LowLayerSubgroups` and minor rephrasing.

Also redid part of gap-system#683, as for some reason (probably my stupidity)
part of gap-system#683 had fallen out of master. Added it again.
@fingolfin fingolfin changed the title Document LowIndexSubgroups ? RootFFE? Document RootFFE? Mar 21, 2018
@fingolfin
Copy link
Member Author

LowIndexSubgroups was documented in PR #2253. This leaves RootFFE which I'd really like to see documented in GAP 4.9.

@fingolfin fingolfin added this to the GAP 4.9.1 milestone Mar 21, 2018
@fingolfin
Copy link
Member Author

By chance, I just learned that RootFFE actually duplicates the functionality of NthRoot, for which we install a method in lib/ffe.gi, albeit one less efficient (I think) than what RootFFE does.

Of course there is then also an operation Root -- but it is "broken" in that it only accepts a multiplicative element and (optional) a positive integer n, but not a domain. I guess one could fix that, and install methods for it that mirror NthRoot etc. ... hohum.

Of course Root is also undocumented, in particular its semantics are unclear. Since it is declared right next to Sqrt, one might expect that Sqrt(x) = Root(x, 2), but this is not the case, as the only method the GAP library installs for Root is RootInt, which returns the integral part of a root... In particular:

gap> Sqrt(5);
E(5)-E(5)^2-E(5)^3+E(5)^4
gap> Root(5,2);
2
gap> Sqrt(-1);
E(4)
gap> Root(-1,2);
Error, Root: <n> is negative but <k> is even in
...

sigh

@fingolfin fingolfin changed the title Document RootFFE? Document RootFFE? Merge it with NthRoot? What about Root? Aug 14, 2018
@olexandr-konovalov olexandr-konovalov modified the milestones: GAP 4.9.3, GAP 4.9.4 Sep 1, 2018
@olexandr-konovalov olexandr-konovalov modified the milestones: GAP 4.9.4, GAP 4.11 Jan 26, 2019
@fingolfin fingolfin added kind: discussion discussions, questions, requests for comments, and so on and removed kind: discussion discussions, questions, requests for comments, and so on kind: question labels Mar 21, 2019
@fingolfin fingolfin removed this from the GAP 4.11 milestone Mar 24, 2019
@fingolfin
Copy link
Member Author

Let's see...

  • NthRoot

    • used in the library: no
    • packages using it: forms
    • documented: no
    • declaration is intended to apply to all fields (but only a method for FFEs is provided)
  • RootFFE

    • used in the library: no
    • packages using it: none
    • documented: no
    • by its name only applies to finite fields, and fits in with LogFFE (and that nicely parallels RootInt and LogInt)
  • Root

    • used in the library: no
    • packages using it: alnuth, radiroot rcwa, unipot
    • documented: no
    • design flaw: is missing a ring/domain argument (but that could be fixed)
    • design flaw: has unclear semantics, e.g. Root(5,2) returns 2 (see my earlier comment for more details)
  • SquareRoots(slightly different scope)

    • used in the library: yes (in lib/ctblgrp.gi)
    • packages using it: recog
    • documented: yes

I guess we could add in Sqrt, RootInt, RootMod, but those are all more limited

@AnnaKDS AnnaKDS linked a pull request May 12, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: discussion discussions, questions, requests for comments, and so on topic: documentation Issues and PRs related to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants