Added Documentation for DecomPoly and NormalizerViaRadical #2360

hulpke · 2018-04-13T14:17:00Z

Apart from a minor syntax change, this PR installs DecomPoly and NormalizerViaRadical as proper functions and adds basic documentation.

This comes as a PR, since git told me I had to do a PR and could not just push the changes.

codecov · 2018-04-13T15:17:17Z

Codecov Report

Merging #2360 into master will increase coverage by 0.21%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #2360      +/-   ##
==========================================
+ Coverage   73.21%   73.42%   +0.21%     
==========================================
  Files         482      482              
  Lines      246200   246196       -4     
==========================================
+ Hits       180251   180778     +527     
+ Misses      65949    65418     -531

Impacted Files	Coverage Δ
lib/norad.gi	`44.67% <ø> (+44.67%)`	⬆️
lib/algfld.gi	`45.16% <87.5%> (+15.23%)`	⬆️
lib/ratfunul.gi	`74.64% <0%> (+0.15%)`	⬆️
lib/arith.gi	`63.46% <0%> (+0.64%)`	⬆️
lib/fitfree.gi	`55.78% <0%> (+4.13%)`	⬆️

fingolfin · 2018-04-13T23:20:36Z

We disabled direct pushes to master months ago; all changes have to go through review (and automatic testing).

fingolfin · 2018-04-13T23:25:07Z

lib/algfld.gd

+##  called an ideal decomposition. In the context of field
+##  extensions, if <M>\alpha</M> is a root of <M>f</M> in a suitable extension
+##  and <M>Q</M> the field of rational numbers, such a decomposition corresponds
+##  to (proper) subfields <M>Q\lt Q(\beta)\lt Q(\alpha)</M>, where <M>g</M> is the


"such a decomposition" vs. "subfields" -- shouldn't either both be in plural, or both be in singular?
I guess technically you can argue that of course Q and Q(\alpha) are also subfields of Q(\alpha), but I still find this mildly irritating to read... But really only mildly, I think I got the gist, thanks :-)

fingolfin · 2018-04-13T23:26:37Z

lib/algfld.gd

+##  decomposition exists). If the option <A>onlyone</A> is given it returns at
+##  most one such decomposition (and performs faster).
+##  <Example><![CDATA[
+##  gap> x:=X(Rationals,"x");;pol:=x^8-24*x^6+144*x^4-288*x^2+144;;


Offtopic: the reference manual says this on X:

X is simply a synonym for Indeterminate. However, we do not recommend to use this synonym which is supported only for the backwards compatibility.

Now, don't get me wrong, I am not suggesting you should change the examples; rather, I always use X myself, so I am wondering if this last sentence in the manual should just be deleted?

Yes. (At the time when this was written there was a push to make X obsolete as operation.)

fingolfin · 2018-04-13T23:27:52Z

lib/algfld.gd

@@ -188,3 +188,45 @@ DeclareGlobalFunction("AlgExtEmbeddedPol");

 DeclareGlobalFunction("AlgExtSquareHensel");

+#############################################################################
+##
+#F  DecomPoly( <f> [:"onlyone"] )  finds ideal decompositions of rational f


Open question: should the name of this function stay DecomPoly? Wouldn't e.g. DecompositionsOfPolynomial more "GAPish"? I don't mind much either way, but once we document the name, we are more or less stuck with it, so I just want this to be a conscious decision.

If we want to change it, then IdealDecompositionsOfPolynomial would be the longish name.

If we document it, I'd kind of prefer the longish name (a shorter alias can still be added). That said, I don't terribly mind if you greatly prefer the shorter name.

fingolfin · 2018-04-13T23:29:04Z

lib/algfld.gd

+##  [ [ x^2+72*x+144, x^6-20*x^4+60*x^2-36 ] ]
+##  ]]></Example>
+##  In this example the given polynomial is regular with Galois group
+##  <M>Q_8</M>, thus exposing the four proper subfields.


I don't understand the last part of this sentence. How does the fact that the Galois group is Q_8 "expose" the four proper subfields?

Q8 is the only group of order 8 with four proper subgroups.

With that hint, I see what you are getting at, but I still don't see how that sentence possibly could convey it... How about this formulation instead:

In this example the given polynomial is regular with Galois group Q_8, which has exactly four proper subgroups, and hence four proper subfields, which are exposed by the decompositions we obtained.

fingolfin · 2018-04-13T23:30:07Z

lib/grp.gd

+##  gap> g:=TransitiveGroup(30,2030);;
+##  gap> s:=SylowSubgroup(g,5);;
+##  gap> Size(NormalizerViaRadical(g,s));
+##  28800


Sounds good to me.

This example of course is slower than the backtrack Normalizer. I have yet to find an example that is while being short enough for the manual.

That's OK, I think (though we might want to mention that the given example is not actually one where NormalizerViaRadical is faster?)

The function thus provided as a
there is an is missing here.

Added a line about example and inserted is.

ssiccha · 2018-04-15T20:21:48Z

Just a general question: Is there any rough complexity estimate known for that algorithm @hulpke?

hulpke · 2018-04-17T17:44:04Z

@ssiccha
The worst case cost (regular Galois group 2^n) is exponential in the input, as the output is.

As for best complexity there is a paper by Klüners, van Hoeij and Belabas (I think) that presents a better method (that is harder to implement).

fingolfin

Other than a typo, this looks good. Ideally, fix that typo, then push again, then we can merge this. Thanks!

fingolfin · 2018-04-17T19:13:03Z

lib/grp.gd

 ##  non-automated tool for advanced users.
 ##  <Example><![CDATA[
 ##  gap> g:=TransitiveGroup(30,2030);;
 ##  gap> s:=SylowSubgroup(g,5);;
 ##  gap> Size(NormalizerViaRadical(g,s));
 ##  28800
 ##  ]]></Example>
+##  Note that this example only demonstartes usage, but that in this case


Typo: demonstartes -> demonstrates

olexandr-konovalov · 2018-11-01T15:58:43Z

This is not what the title says. This results in new IdealDecompositionsOfPolynomial being documented, but not DecomPoly.

hulpke requested a review from fingolfin April 13, 2018 14:30

fingolfin reviewed Apr 13, 2018

View reviewed changes

fingolfin mentioned this pull request Apr 15, 2018

doc: don't recommend avoiding X #2366

Merged

fingolfin added topic: documentation Issues and PRs related to documentation topic: library labels Apr 16, 2018

fingolfin added this to the GAP 4.10.0 milestone Apr 16, 2018

hulpke force-pushed the master branch from 21f5e46 to b78f594 Compare April 17, 2018 17:45

fingolfin approved these changes Apr 17, 2018

View reviewed changes

hulpke added 2 commits April 17, 2018 14:29

Added documentation for NormalizerViaRadical.

107dbd5

Added documentation (and minor fixes) for DecomPoly.

444be92

hulpke force-pushed the master branch from b78f594 to 444be92 Compare April 17, 2018 20:29

fingolfin mentioned this pull request Apr 17, 2018

Remove lib/norad.gi with NormalizerViaRadical code #2282

Closed

fingolfin merged commit 8ff53d6 into gap-system:master Apr 17, 2018

fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Sep 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Documentation for DecomPoly and NormalizerViaRadical #2360

Added Documentation for DecomPoly and NormalizerViaRadical #2360

hulpke commented Apr 13, 2018

codecov bot commented Apr 13, 2018 •

edited

Loading

fingolfin commented Apr 13, 2018

fingolfin Apr 13, 2018

fingolfin Apr 13, 2018

hulpke Apr 14, 2018

fingolfin Apr 13, 2018

hulpke Apr 15, 2018

fingolfin Apr 15, 2018

fingolfin Apr 13, 2018

hulpke Apr 14, 2018

fingolfin Apr 15, 2018

fingolfin Apr 13, 2018

hulpke Apr 14, 2018

fingolfin Apr 15, 2018

ssiccha Apr 15, 2018

hulpke Apr 17, 2018

ssiccha commented Apr 15, 2018

hulpke commented Apr 17, 2018

fingolfin left a comment

fingolfin Apr 17, 2018

olexandr-konovalov commented Nov 1, 2018

Added Documentation for DecomPoly and NormalizerViaRadical #2360

Added Documentation for DecomPoly and NormalizerViaRadical #2360

Conversation

hulpke commented Apr 13, 2018

codecov bot commented Apr 13, 2018 • edited Loading

Codecov Report

fingolfin commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssiccha commented Apr 15, 2018

hulpke commented Apr 17, 2018

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olexandr-konovalov commented Nov 1, 2018

codecov bot commented Apr 13, 2018 •

edited

Loading