sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.33k stars 453 forks source link

Fix random order of cubic braid group elements #33498

Closed soehms closed 2 years ago

soehms commented 2 years ago

Currently the order of cubic braid group elements is realized via the Gap functionality for finitely presented groups in cases where the number of strands is less than 5.

According to the Gap Reference manual (see section 47.3-2) it is not guaranteed that this gives constant results.

Therefore, we should order the elements independent on the number of strands according to their associated matrix.

CC: @tscrim

Component: group theory

Keywords: order cubic braid random

Author: Sebastian Oehms

Branch/Commit: f2288a7

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/33498

soehms commented 2 years ago

Branch: u/soehms/fix_random_order_cubic_braids_33498

soehms commented 2 years ago

Commit: b7dba2f

soehms commented 2 years ago

New commits:

b7dba2f33498: initial version
soehms commented 2 years ago

Author: Sebastian Oehms

tscrim commented 2 years ago
comment:3

This change is good overall. However, the patchbot reports one failure:

sage -t --long --random-seed=73985986695613081951621256359794760011 src/sage/groups/cubic_braid.py
**********************************************************************
File "src/sage/groups/cubic_braid.py", line 1553, in sage.groups.cubic_braid.CubicBraidGroup.as_permutation_group
Failed example:
    C3.is_isomorphic(PC3)
Expected:
    True
Got:
    #I  Forcing finiteness test
    True
**********************************************************************

A while-we-are-at-it would be to change the _richcmp_ docstring to start with a capital letter and have a proper one-line description like

Rich comparison of ``self`` with ``other``.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

a64a3fe33498: changes according to review
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from b7dba2f to a64a3fe

soehms commented 2 years ago
comment:5

Replying to @tscrim:

This change is good overall. However, the patchbot reports one failure...

I can't reproduce this. It neither shows in the Gitpod environment:

gitpod /workspace/sagetrac-mirror (u/soehms/fix_random_order_cubic_braids_33498) $ ./sage -tp src/sage/groups/cubic_braid.py
too few successful tests, not using stored timings
Running doctests with ID 2022-03-15-12-50-51-47dccf3b.
Git branch: u/soehms/fix_random_order_cubic_braids_33498
Using --optional=debian,pip,sage,sage_spkg
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,nauty,palp,pandoc,pdf2svg,pdftocairo,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Doctesting 1 file using 8 threads.
sage -t --random-seed=106186706550360108543211691833118152182 src/sage/groups/cubic_braid.py
    [186 tests, 6.74 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 6.9 seconds
    cpu time: 7.3 seconds
    cumulative wall time: 6.7 seconds

It seems to be something sporadic coming from these lines in Gap:

#############################################################################
##
#F  IsomorphismGroups(<G>,<H>) . . . . . . . . . .  isomorphism from G onto H
##
InstallGlobalFunction(IsomorphismGroups,function(G,H)
local m;

  if not HasIsFinite(G) or not HasIsFinite(H) then
    Info(InfoWarning,1,"Forcing finiteness test");
    IsFinite(G);
    IsFinite(H);
  fi;
  if not IsFinite(G) and not IsFinite(H) then
    Error("cannot test isomorphism of infinite groups");
  fi;
  if IsFinite(G) <> IsFinite(H) then
    return fail;
  fi;

Apparently this info message shows in a couple of doctests:

sebastian@TP-OEHMS:~/devel/sage$ git grep "#I  Forcing finiteness test"
src/sage/groups/cubic_braid.py:        #I  Forcing finiteness test
src/sage/groups/finitely_presented.py:            #I  Forcing finiteness test
src/sage/groups/finitely_presented.py:            #I  Forcing finiteness test
src/sage/groups/finitely_presented.py:            #I  Forcing finiteness test
src/sage/groups/finitely_presented.py:            #I  Forcing finiteness test
src/sage/groups/finitely_presented_named.py:        #I  Forcing finiteness test
src/sage/groups/finitely_presented_named.py:        #I  Forcing finiteness test
src/sage/groups/finitely_presented_named.py:        #I  Forcing finiteness test
src/sage/groups/finitely_presented_named.py:        #I  Forcing finiteness test
src/sage/groups/finitely_presented_named.py:        #I  Forcing finiteness test
src/sage/groups/finitely_presented_named.py:        #I  Forcing finiteness test
src/sage/groups/finitely_presented_named.py:        #I  Forcing finiteness test

Furthermore, I have no idea how this can be connected with the branch of the ticket.

I marked the test as random since I don't have a better idea.

tscrim commented 2 years ago
comment:6

Since it is a True/False output, you can change it to

sage: assert C3.is_isomorphic(PC3)  # random (with respect to the occurrence of the info message)
#I  Forcing finiteness test

which will fail if it is False as # random does not catch error messages, whereas the current test will hide the error. (Also note occurence -> occurrence typo.)

I don't see what makes this particular one machine dependent as I also cannot locally reproduce either. shrugs

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

f2288a733498: once again concerning random doctest
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from a64a3fe to f2288a7

soehms commented 2 years ago
comment:8

Replying to @tscrim:

Since it is a True/False output, you can change it to ...

Thanks for the hint!

tscrim commented 2 years ago

Reviewer: Travis Scrimshaw

tscrim commented 2 years ago
comment:9

Thanks. LGTM.

vbraun commented 2 years ago

Changed branch from u/soehms/fix_random_order_cubic_braids_33498 to f2288a7