Document the limits of bipartitions #296

ChristopherRussell · 2017-05-31T11:19:28Z

Added two sentences to the doc explaining that bipartitions are capped at degree 2 ^ 31 yet are unlikely to work (due to memory constraints) for even smaller degrees.
Added errors for methods within bipart.gi which bipartitions. These occur detect when the user attempts to create a bipartition of degree greater than or equal to 2 ^ 31.
Covered these new error messages with new tests.

mtorpey · 2017-05-31T11:35:14Z

gap/elements/bipart.gi

@@ -241,6 +262,11 @@ InstallMethod(RandomBlockBijection, "for a random source and pos int",
 function(rs, n)
  local out, nrblocks, j, free, i;

+  if n >= 2^31 then


The linter will complain about this - put whitespace around ^.

mtorpey · 2017-05-31T11:36:09Z

gap/elements/bipart.gi

@@ -93,6 +93,14 @@ InstallGlobalFunction(Bipartition,
 function(classes)
  local n, copy, i, j;

+  n := Sum(List(classes, Length)) / 2;


In fact, Sum(List(classes, Length)) can be replaced by simply Sum(classes, Length). I believe the same goes for Product. Handy, eh?

james-d-mitchell

Looks pretty good, few minor changes and we'll merge it.

james-d-mitchell · 2017-05-31T14:38:08Z

doc/z-chap03.xml

-  described in this section.
+  described in this section. The maximum degree of a bipartition is set as 
+  2 ^ 31. In reality, bipartitions of degrees as small as 2 ^ 24 are unlikely
+  to work because they require too much memory.


"In reality, it is unlikely to be possible to create bipartitions of degrees as small as 2 ^ 24 because they require too much memory."

Also it seems that you limit this to 2 ^ 31 - 1 so this comment is not very accurate.

james-d-mitchell · 2017-05-31T14:40:00Z

gap/elements/bipart.gi

+  if n >= 2 ^ 31 then
+    ErrorNoReturn("Semigroups: Bipartition: usage,\n",
+                  "the argument <classes> must be a list of lists whose union",
+                  "has length at most 2 ^ 31 - 1,");


shouldn't this be "consists of values in the range 1 to 2 ^ 31 - 1"? Since if it is a list of length 10 containing numbers in excess of 2 ^ 31 - 1, then it will not work.

I that think the function takes a list of lists as its argument and the degree is half the length of the union of the lists.

Yes, but as you've written it the numbers in the lists can be arbitrarily big as long as there are less than 2 ^ 31 - 1 of them.

james-d-mitchell · 2017-05-31T14:42:28Z

tst/standard/bipart.tst

@@ -484,8 +484,8 @@ gap> RightProjection(StarOp(x));

 # bipartition: Bipartition 1/3
 gap> Bipartition("test");
-Error, Semigroups: Bipartition: usage,
-the argument <classes> must consist of duplicate-free homogeneous lists,


It looks like the changes in this pull request turn an informative error (the previous one) into an uninformative error (the current one). Please revert this.

james-d-mitchell · 2017-05-31T14:43:06Z

gap/elements/bipart.gi

@@ -93,6 +93,14 @@ InstallGlobalFunction(Bipartition,
 function(classes)
  local n, copy, i, j;

+  n := Sum(classes, Length) / 2;


This can't go here, since you do not yet know that classes consists of lists.

mtorpey · 2017-06-02T14:50:55Z

32-bit tests are failing - we have no idea why, but it's something in the very first commit in this PR.

Looks like something weird.

We'll fix this later.

james-d-mitchell · 2018-03-27T09:30:46Z

I wonder if this is now resolved by the resolution of #444, I'll run travis again and see what happens.

james-d-mitchell · 2018-03-27T09:32:57Z

No wait, it'll first need to be rebased. @ChristopherRussell can you please rebase this onto stable-3.0? If this still fails, then I guess the reason is that we somehow create a too long list in 32-bit mode in the kernel module.

james-d-mitchell · 2018-04-03T09:41:16Z

tst/standard/bipart.tst

 Error, Semigroups: Bipartition: usage,
 the argument <classes> must be a list of lists whose unionhas length at most 2\
 ^ 31 - 1,
-gap> BipartitionByIntRep([1 .. 2 ^ 32]);
+gap> BipartitionByIntRep(enum);
 Error, Semigroups: BipartitionByIntRep: usage,
 the length of the argument <blocks> must not exceed2 ^ 32 - 1,


Seems to be a space missing in the error message here.

james-d-mitchell · 2018-04-03T09:41:56Z

gap/elements/bipart.gi

+  if n >= 2 ^ 32 then
+    ErrorNoReturn("Semigroups: BipartitionByIntRep: usage,\n",
+                  "the length of the argument <blocks> must not exceed",
+                  "2 ^ 32 - 1,");


Missing space.

james-d-mitchell · 2018-04-03T09:43:35Z

Did you rebase this already @ChristopherRussell ? I can't tell, so please comment when you have done it.

ChristopherRussell · 2018-04-03T09:59:46Z

I have just rebased it (and included the extra space in the error output / test output which you recently suggested)

ChristopherRussell · 2018-04-03T10:20:28Z

@james-d-mitchell Travis 32-bit build is giving the following error:

testing: /home/travis/gap/pkg/semigroups/tst/standard/bipart.tst
######## > Diff in:
/home/travis/gap/pkg/semigroups/tst/standard/bipart.tst:107
# Input is:
ForAll(elts, x -> AsBipartition(AsPartialPerm(x), 3) = x);
# Expected output:
true
# But found:
false
########

where elts is

elts := Filtered(PartitionMonoid(3), IsPartialPermBipartition);;

This doesn't occur on my computer, any ideas what the difference is with the 32-bit mode?

james-d-mitchell · 2018-04-03T11:13:46Z

I think I know the reason, I'll try to fix it now.

james-d-mitchell · 2018-04-03T11:26:08Z

Actually, maybe not. Just to double check @ChristopherRussell are you sure that you pulled from the master branch before rebasing? I ask because the 32-bit tests also include a "function must return a value" error, which is the same error as in Issue #444, which was caused by a (now resolved) bug in the bipartitions code.

james-d-mitchell · 2018-04-03T11:30:54Z

Looking at the diff in the files, you may have rebased this on the current master branch, but there are changes to some files which I don't think should be there, such as in .clang-format, bipart.cc, and so on. Can you please undo the changes to those files which are formatting/whitespace changes only?
@ChristopherRussell

james-d-mitchell · 2018-04-03T11:31:21Z

src/.clang-format

@@ -1,11 +1,10 @@
 ---


Please do not commit any changes to this file.

ChristopherRussell · 2018-04-03T11:32:39Z

I think I did make a mistake when trying to update my local version of stable-3.0 before rebasing

wilfwilson · 2018-04-03T11:36:19Z

This is currently a PR against master, but shouldn't it be against stable-3.0? Chris's branch is based on stable-3.0.

…me doc.

ChristopherRussell · 2018-04-03T12:08:46Z

As Wilf said I was rebasing against master when I meant to rebase against stable-3.0 which cased the changes to kernel code. However, there is still error in the 32-bit build tests:

testing: /home/travis/gap/pkg/semigroups/tst/standard/bipart.tst
######## > Diff in:
/home/travis/gap/pkg/semigroups/tst/standard/bipart.tst:107
# Input is:
ForAll(elts, x -> AsBipartition(AsPartialPerm(x), 3) = x);
# Expected output:
true
# But found:
false
########

where elts is

elts := Filtered(PartitionMonoid(3), IsPartialPermBipartition);;

and this doesn't seem to be resolved by the fix to Issue #444 as @james-d-mitchell suggested.

This occasionally caused incorrect values to be returned when i == deg and by coincidence ptf2[i] == cpt. When ptf2[deg] is beyond the end of the bag containing f, and so is not valid. For reference, this bug caused errors in: semigroups/Semigroups#296 semigroups/Semigroups#472

james-d-mitchell · 2018-04-05T09:46:03Z

I'm going to close this in favour of PR #472.

This occasionally caused incorrect values to be returned when i == deg and by coincidence ptf2[i] == cpt. When ptf2[deg] is beyond the end of the bag containing f, and so is not valid. For reference, this bug caused errors in: semigroups/Semigroups#296 semigroups/Semigroups#472

mtorpey reviewed May 31, 2017

View reviewed changes

mtorpey added the 3.0 label May 31, 2017

ChristopherRussell force-pushed the bipartdoc branch 5 times, most recently from 6756ee5 to c892353 Compare May 31, 2017 14:28

james-d-mitchell requested changes May 31, 2017

View reviewed changes

wilfwilson changed the title ~~Limits of Bipartitions~~ Document the limits of bipartitions May 31, 2017

mtorpey added 3.1 and removed 3.0 labels Jun 2, 2017

mtorpey added the do not merge Label for PR that should not be merged label Jun 14, 2017

james-d-mitchell added bug Label for issues or PR which report or fix bugs 3.0 and removed 3.1 labels Mar 27, 2018

james-d-mitchell approved these changes Mar 27, 2018

View reviewed changes

james-d-mitchell reviewed Apr 3, 2018

View reviewed changes

ChristopherRussell force-pushed the bipartdoc branch 2 times, most recently from 8a27151 to 91b80ae Compare April 3, 2018 09:58

james-d-mitchell reviewed Apr 3, 2018

View reviewed changes

src/.clang-format Outdated

@@ -1,11 +1,10 @@

---

Copy link

Collaborator

james-d-mitchell Apr 3, 2018

Choose a reason for hiding this comment

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

Please do not commit any changes to this file.

ChristopherRussell added 4 commits April 3, 2018 12:40

Added checks to ensure bipartitions have degree at most 2^31-1 and so…

f290397

…me doc.

Added tests to cover new error messages.

62d68a6

bipart.tst: fix to new tests for 32-bit gap.

ae5ef51

Fixes for PR: Bipartition error output. Doc wording.

7d985bf

ChristopherRussell changed the base branch from master to stable-3.0 April 3, 2018 11:43

ChristopherRussell force-pushed the bipartdoc branch from 91b80ae to 7d985bf Compare April 3, 2018 11:46

james-d-mitchell removed the bug Label for issues or PR which report or fix bugs label Apr 3, 2018

james-d-mitchell mentioned this pull request Apr 4, 2018

bipart: doc and enforce degree limit of 2 ^ 29 - 1 #472

Merged

james-d-mitchell closed this Apr 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document the limits of bipartitions #296

Document the limits of bipartitions #296

ChristopherRussell commented May 31, 2017 •

edited by james-d-mitchell

Loading

mtorpey May 31, 2017

mtorpey May 31, 2017

james-d-mitchell left a comment

james-d-mitchell May 31, 2017

james-d-mitchell May 31, 2017

james-d-mitchell May 31, 2017

ChristopherRussell May 31, 2017

james-d-mitchell Jun 1, 2017

james-d-mitchell May 31, 2017

james-d-mitchell May 31, 2017

mtorpey commented Jun 2, 2017

james-d-mitchell commented Mar 27, 2018

james-d-mitchell commented Mar 27, 2018 •

edited

Loading

james-d-mitchell Apr 3, 2018

james-d-mitchell Apr 3, 2018

james-d-mitchell commented Apr 3, 2018

ChristopherRussell commented Apr 3, 2018 •

edited

Loading

ChristopherRussell commented Apr 3, 2018 •

edited

Loading

james-d-mitchell commented Apr 3, 2018

james-d-mitchell commented Apr 3, 2018

james-d-mitchell commented Apr 3, 2018 •

edited

Loading

james-d-mitchell Apr 3, 2018

ChristopherRussell commented Apr 3, 2018

wilfwilson commented Apr 3, 2018 •

edited

Loading

ChristopherRussell commented Apr 3, 2018 •

edited

Loading

james-d-mitchell commented Apr 5, 2018

Document the limits of bipartitions #296

Document the limits of bipartitions #296

Conversation

ChristopherRussell commented May 31, 2017 • edited by james-d-mitchell Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-d-mitchell left a 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

mtorpey commented Jun 2, 2017

james-d-mitchell commented Mar 27, 2018

james-d-mitchell commented Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-d-mitchell commented Apr 3, 2018

ChristopherRussell commented Apr 3, 2018 • edited Loading

ChristopherRussell commented Apr 3, 2018 • edited Loading

james-d-mitchell commented Apr 3, 2018

james-d-mitchell commented Apr 3, 2018

james-d-mitchell commented Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

ChristopherRussell commented Apr 3, 2018

wilfwilson commented Apr 3, 2018 • edited Loading

ChristopherRussell commented Apr 3, 2018 • edited Loading

james-d-mitchell commented Apr 5, 2018

ChristopherRussell commented May 31, 2017 •

edited by james-d-mitchell

Loading

james-d-mitchell commented Mar 27, 2018 •

edited

Loading

ChristopherRussell commented Apr 3, 2018 •

edited

Loading

ChristopherRussell commented Apr 3, 2018 •

edited

Loading

james-d-mitchell commented Apr 3, 2018 •

edited

Loading

wilfwilson commented Apr 3, 2018 •

edited

Loading

ChristopherRussell commented Apr 3, 2018 •

edited

Loading