Add Display method for pc groups #3621

flyingapfopenguin · 2019-08-23T10:06:43Z

Description / Text for release notes

Add Display method for pc groups, refering #3538.

stevelinton · 2019-08-23T11:31:04Z

Is there anything to be said for omitting relations of the form g17^g15 = g17 from the printout and instead appending all other pairs of generators commute. It is quite common to have presentations which are "sparse" in the sense that most pairs of generators commute.

codecov · 2019-08-23T11:44:05Z

Codecov Report

Merging #3621 into master will increase coverage by 0.07%.
The diff coverage is 96.61%.

@@            Coverage Diff             @@
##           master    #3621      +/-   ##
==========================================
+ Coverage   84.58%   84.65%   +0.07%     
==========================================
  Files         698      698              
  Lines      345626   345685      +59     
==========================================
+ Hits       292341   292637     +296     
+ Misses      53285    53048     -237

Impacted Files	Coverage Δ
lib/grppc.gd	`100% <100%> (ø)`	⬆️
lib/grppc.gi	`81.27% <96.55%> (+0.55%)`	⬆️
src/weakptr.c	`96.44% <0%> (-0.4%)`	⬇️
src/opers.c	`94.79% <0%> (-0.07%)`	⬇️
lib/groebner.gi	`85.72% <0%> (+0.1%)`	⬆️
lib/streams.gi	`74.28% <0%> (+0.1%)`	⬆️
lib/combinat.gi	`77.18% <0%> (+0.12%)`	⬆️
lib/matrix.gi	`87.74% <0%> (+0.29%)`	⬆️
lib/grp.gi	`89.51% <0%> (+0.31%)`	⬆️
lib/vec8bit.gi	`80.39% <0%> (+0.42%)`	⬆️
... and 19 more

flyingapfopenguin · 2019-08-23T11:47:31Z

@stevelinton Thanks for the reply. I added the message to the output of the Display function.

wilfwilson · 2019-08-26T10:06:30Z

The tests need to be updated to account for the new line that you added.

Also, I think that the line All other pairs of generators commute. should only be shown if there are actually such pairs of generators that commute. (I don't know anything about pc groups really, so perhaps it is possible that there are always some generators that commute).

fingolfin · 2019-08-26T21:54:53Z

@flyingapfopenguin you added the message, but did not update the tests (which you can do by running Test("tst/PATH/TO/TEST.tst", rec(rewriteToFile:=true));

lib/grppc.gi

tst/testinstall/grppc.tst

lib/grppc.gi

flyingapfopenguin · 2019-08-27T13:21:50Z

@wilfwilson @fingolfin Thanks for your constructive response. I applied (most of) your suggestions.

tst/testinstall/grppc.tst

lib/grppc.gi

DominikBernhardt · 2019-08-28T08:13:02Z

lib/grppc.gi

+    gens := GeneratorsOfGroup( F );
+    pis  := RelativeOrders( pcgs );
+
+    # the power


Could you maybe improve this comment so it says more about the following code?

I tried to improve it a little. Do you want more?

DominikBernhardt · 2019-08-28T09:03:42Z

One general remark: You print the message All other pairs of generators commute but the user never sees the generators. Should one maybe print the number of generators for the pc group as well?

coveralls · 2019-08-29T19:41:25Z

Coverage increased (+0.01%) to 84.419% when pulling 5d4c5fc on flyingapfopenguin:jb/DisplayPCGroup into df36d37 on gap-system:master.

lib/grppc.gd

fingolfin · 2019-08-29T22:23:02Z

lib/grppc.gi

+    local pcgs, n, F, gens, i, pis, exp, t, h, rel, commPower, j, trivialCommutators;
+
+    pcgs:=Pcgs(G);
+    n:=Length(pcgs);


This is not a request for change, just wanted to point something out: the above two lines clearly come from another source than the following three lines, based on their formatting... Overall, this function mixes several code formatting styles (e.g. Foo( arg ) vs. Foo(arg))

@fingolfin What is the GAP coding style? As I didn't knew I copied code without changing its formating and coded the rest like it seems to make sense to me.

There is no official coding style (and trying to set one is one of these things were I expect a lot of pain and blood for little gain). Many people seem to take the formatting which GAP itself uses when printing a function as reference, but usually with some adjustments (e.g. aligning some things across multiple lines, as below).

Anyway, I am not asking for a specific style to be followed -- just that the code within a given function be self-consistent. That's note the case here.

I'm going to fix this tomorrow by adding a new commit, so that you can do a reintegration merge afterwards.

I already merged this PR, didn't want to hold it hostage over some whitespace differences :-) I only pointed this formatting bit out again to make you aware of it; as it is, lots of GAP code is less well formatted than what is in this PR...

fingolfin · 2019-08-29T22:26:24Z

lib/grppc.gi

+        fi;
+        n := Size(Pcgs(G));
+        if IsOne(n) then
+            Print("cyclic pc-group with one pc-generator and the relation:\n");


Unimportant nitpick: So you print "one pc-generator" but "2 pc-generators" ? maybe "one" -> "1" for consistency?

I am not sure I see the importance of adding cyclic. It might even be misleading, because many cyclic pc-groups won't get it (e.g. C_4 or C_6 if the pcgs is refined).

I'm going to change "one" to "1" for consistency.

About "cyclic": I don't think this is important in any way but i'm going to leave this.

fingolfin · 2019-08-29T22:27:38Z

lib/grppc.gi

+        fi;
+        trivialCommutators := PrintPcPresentation( G, false );
+        if IsAbelian(G) then
+          Print("all generators commute, the groups is abelian\n");


Again unimportant, but: If you handle the 1 generator case separately above, you might want to consider doing so here as well, as "all generators" then refers to just 1 generator.

That's right, but all is one in this case.

fingolfin

Overall seems fine to me now, though I had some further minor nitpicks. The PR should also be squashed.

flyingapfopenguin · 2019-08-30T13:51:39Z

Overall seems fine to me now, though I had some further minor nitpicks. The PR should also be squashed.

Should I simply squash all commits together and do a force push or how is this usually done?
If I anyhow do a force push, should I also rebase on the current master?

fingolfin · 2019-09-02T07:47:40Z

@flyingapfopenguin yes, please squash, rebase on latest master and then force push.

flyingapfopenguin · 2019-09-02T15:01:14Z

@flyingapfopenguin yes, please squash, rebase on latest master and then force push.

done

DominikBernhardt · 2019-09-03T13:21:15Z

One test failed due to a download error, I restarted it.

wucas · 2019-09-10T13:35:45Z

Is there a reason why this PR does not close Issue #3538?

DominikBernhardt · 2019-09-11T02:35:40Z

Is refering maybe not the right keyword? Could be that one has to write resolves or fixes? I can close the original issue manually this time.

flyingapfopenguin added gapdays2019-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2019-spring kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements labels Aug 23, 2019

flyingapfopenguin requested review from fingolfin and DominikBernhardt August 23, 2019 11:23

wilfwilson added the topic: library label Aug 26, 2019

fingolfin reviewed Aug 26, 2019

View reviewed changes

fingolfin reviewed Aug 27, 2019

View reviewed changes

tst/testinstall/grppc.tst Outdated Show resolved Hide resolved

fingolfin reviewed Aug 27, 2019

View reviewed changes

lib/grppc.gi Outdated Show resolved Hide resolved

DominikBernhardt reviewed Aug 28, 2019

View reviewed changes

lib/grppc.gi Outdated Show resolved Hide resolved

DominikBernhardt reviewed Aug 28, 2019

View reviewed changes

flyingapfopenguin force-pushed the jb/DisplayPCGroup branch from efb6f5a to 9bc1ba2 Compare August 28, 2019 13:36

flyingapfopenguin requested a review from fingolfin August 29, 2019 11:44

fingolfin reviewed Aug 29, 2019

View reviewed changes

fingolfin approved these changes Aug 29, 2019

View reviewed changes

Add Display method for pc groups

5d4c5fc

flyingapfopenguin force-pushed the jb/DisplayPCGroup branch from 00cb8e6 to 5d4c5fc Compare September 2, 2019 14:58

DominikBernhardt closed this Sep 3, 2019

DominikBernhardt reopened this Sep 3, 2019

fingolfin merged commit e745d3c into gap-system:master Sep 3, 2019

fingolfin added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Sep 3, 2019

wucas mentioned this pull request Sep 10, 2019

Add a nice Display method for pc group #3538

Closed

olexandr-konovalov added this to the GAP 4.11.0 milestone Nov 17, 2019

dimpase added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Display method for pc groups #3621

Add Display method for pc groups #3621

flyingapfopenguin commented Aug 23, 2019

stevelinton commented Aug 23, 2019

codecov bot commented Aug 23, 2019 •

edited

Loading

flyingapfopenguin commented Aug 23, 2019

wilfwilson commented Aug 26, 2019

fingolfin commented Aug 26, 2019

flyingapfopenguin commented Aug 27, 2019

DominikBernhardt Aug 28, 2019

flyingapfopenguin Aug 28, 2019

DominikBernhardt commented Aug 28, 2019

coveralls commented Aug 29, 2019 •

edited

Loading

fingolfin Aug 29, 2019

flyingapfopenguin Aug 30, 2019

fingolfin Sep 3, 2019

flyingapfopenguin Sep 3, 2019

fingolfin Sep 3, 2019

fingolfin Aug 29, 2019

flyingapfopenguin Aug 30, 2019

fingolfin Aug 29, 2019

flyingapfopenguin Aug 30, 2019

fingolfin left a comment

flyingapfopenguin commented Aug 30, 2019

fingolfin commented Sep 2, 2019

flyingapfopenguin commented Sep 2, 2019

DominikBernhardt commented Sep 3, 2019

wucas commented Sep 10, 2019

DominikBernhardt commented Sep 11, 2019

Add Display method for pc groups #3621

Add Display method for pc groups #3621

Conversation

flyingapfopenguin commented Aug 23, 2019

Description / Text for release notes

stevelinton commented Aug 23, 2019

codecov bot commented Aug 23, 2019 • edited Loading

Codecov Report

flyingapfopenguin commented Aug 23, 2019

wilfwilson commented Aug 26, 2019

fingolfin commented Aug 26, 2019

flyingapfopenguin commented Aug 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominikBernhardt commented Aug 28, 2019

coveralls commented Aug 29, 2019 • edited Loading

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

fingolfin left a comment

Choose a reason for hiding this comment

flyingapfopenguin commented Aug 30, 2019

fingolfin commented Sep 2, 2019

flyingapfopenguin commented Sep 2, 2019

DominikBernhardt commented Sep 3, 2019

wucas commented Sep 10, 2019

DominikBernhardt commented Sep 11, 2019

codecov bot commented Aug 23, 2019 •

edited

Loading

coveralls commented Aug 29, 2019 •

edited

Loading