`sage.groups.matrix_gps`: Modularization fixes for imports by mkoeppe · Pull Request #35306 · sagemath/sage

mkoeppe · 2023-03-18T23:47:00Z

📚 Description

We move the ...MatrixGroup_gap classes to separate modules named ..._gap.

This is part of:

Meta-ticket: Modularize sagelib into separate distributions (pip-installable packages) sagemath-... for Sage 10.x #29705

📝 Checklist

I have made sure that the title is self-explanatory and the description concisely explains the PR.
I have linked an issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

Depends on sage.{coding,groups}: Replace imports from sage.*.all for namespace packages #35099
Depends on Deprecate is_FiniteField etc., make sage.rings.finite_rings a namespace package #35119

…___all_for_namespace_packages' into matrix_groups_modularization

…te ..._gap module

…m .all

…nitefield' into matrix_groups_modularization

…ce of is_MatrixGroup, is_ArithmeticSubgroup

… back to _generic

codecov-commenter · 2023-03-19T07:35:10Z

Codecov Report

Patch coverage: 89.69% and project coverage change: -0.02 ⚠️

Comparison is base (c00e6c2) 88.62% compared to head (c362fb7) 88.61%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35306      +/-   ##
===========================================
- Coverage    88.62%   88.61%   -0.02%     
===========================================
  Files         2148     2155       +7     
  Lines       398855   398915      +60     
===========================================
+ Hits        353480   353484       +4     
- Misses       45375    45431      +56

Impacted Files	Coverage Δ
src/sage/groups/affine_gps/affine_group.py	`82.02% <ø> (-16.86%)`	⬇️
src/sage/groups/affine_gps/euclidean_group.py	`50.00% <ø> (-50.00%)`	⬇️
src/sage/groups/affine_gps/group_element.py	`89.38% <ø> (-5.31%)`	⬇️
src/sage/matrix/matrix_space.py	`89.07% <69.23%> (-0.55%)`	⬇️
src/sage/groups/matrix_gps/orthogonal.py	`92.85% <71.42%> (-4.62%)`	⬇️
src/sage/groups/matrix_gps/unitary.py	`82.08% <71.42%> (-9.70%)`	⬇️
src/sage/groups/matrix_gps/symplectic.py	`88.88% <77.77%> (-8.99%)`	⬇️
src/sage/groups/matrix_gps/linear.py	`84.90% <80.00%> (-6.40%)`	⬇️
src/sage/groups/matrix_gps/finitely_generated.py	`83.33% <87.50%> (-7.18%)`	⬇️
...c/sage/groups/matrix_gps/finitely_generated_gap.py	`89.28% <89.28%> (ø)`
... and 15 more

... and 28 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

…dularization

…random

SageMath version 10.0.beta7, Release Date: 2023-04-01

…ite_rings' when GF is involved, add '# optional - sage.symbolic'

…ted by mistake)

SageMath version 10.0.beta8, Release Date: 2023-04-06

kwankyu · 2023-04-14T06:20:30Z

Looks good in the first pass.

Would you put the boilerplate things like copyright notices in the new files?

…lame -M -C -w --date=format:%Y FILE.py | sort -k2'

mkoeppe · 2023-04-14T18:56:09Z

Done.

src/sage/groups/matrix_gps/matrix_group.py

src/sage/groups/matrix_gps/matrix_group_gap.py

…tors'

src/sage/groups/matrix_gps/orthogonal_gap.py

…sed on 'git blame -M -C -w --date=format:%Y FILE.py | sort -k2'

kwankyu

Thanks.

github-actions · 2023-04-17T03:41:28Z

Documentation preview for this PR is ready! 🎉
Built with commit: 1024a23

mkoeppe · 2023-04-17T06:42:25Z

Thank you!

antonio-rojas · 2023-04-27T16:32:02Z

I'm getting a test failure on Arch with system GAP 4.12.2 after this

**********************************************************************
File "/usr/lib/python3.10/site-packages/sage/groups/matrix_gps/finitely_generated_gap.py", line 124, in sage.groups.matrix_gps.finitely_generated_gap.FinitelyGeneratedMatrixGroup_gap.as_permutation_group
Failed example:
    P == Psmaller
Expected:
    False
Got:
    True
**********************************************************************

Strangely, the exact same test passed when it was in finitely_generated.py

kwankyu · 2023-04-28T05:05:13Z

To examine the failure, I tried to run tox -e docker-archlinux-latest-standard but this failed with

...
DEPRECATED: The legacy builder is deprecated and will be removed in a future release.
            BuildKit is currently disabled; enable it by removing the DOCKER_BUILDKIT=0
            environment-variable.
...
Step 16/44 : RUN sh -x -c "${BOOTSTRAP}"
 ---> Running in 3cf527175f0d
+ ./bootstrap
bootstrap:39: installing 'm4/sage_spkg_configures.m4'
/sage/build/bin/sage-bootstrap-python: error: none of python python3 python3.12 python3.11 python3.10 python3.9 python3.8 python3.7 python2.7 python3.6 python2 is a suitable Python
...

Next I tried to run ci linux workflow on my fork of sage. It also failed (actually all flavors of linux failed)

What did I wrong? I have zero experience in testing sage on linux platforms, by the way.

dimpase · 2023-04-28T11:51:30Z

It's probably a bug in tox file. Anyway, you can just run archlinux in Docker, and build Sage there the usual way.

kwankyu · 2023-04-28T12:18:03Z

Yes. This is a good opportunity for me to try sage on linux.

antonio-rojas · 2023-04-28T14:45:51Z

After some more testing, it seems to be the line

56    sage: TestSuite(G).run()

which triggers this. It is somehow interfering with the random generator status of GAP.
The test

132    sage: Psmaller.degree()                     # random

was marked as # random in c362fb7 (with no given reason), I guess the same should be done for this one.

dimpase · 2023-04-28T15:11:41Z

tagging everything #random is a bit lame. One can reset GAP's RNG before a test, like this:

gap.Reset(gap.GlobalMersenneTwister,"42")
libgap.Reset(libgap.GlobalMersenneTwister,"42")

(this is both for pexpect and libgap interfaces - probably just one of these is needed)

antonio-rojas · 2023-04-28T15:26:15Z

tagging everything #random is a bit lame. One can reset GAP's RNG before a test, like this:
gap.Reset(gap.GlobalMersenneTwister,"42")
libgap.Reset(libgap.GlobalMersenneTwister,"42")
(this is both for pexpect and libgap interfaces - probably just one of these is needed)

That's done in the Psmaller constructor already, so the difference must come from somewhere else.

antonio-rojas · 2023-04-28T15:33:50Z

Things get weirder, it depends on the order in which P and Psmaller are defined:

sage: imf = libgap.function_factory('ImfMatrixGroup')
sage: GG = imf( 12, 3 )
sage: G = MatrixGroup(GG.GeneratorsOfGroup())
sage: P = G.as_permutation_group()
sage: Psmaller = G.as_permutation_group(algorithm="smaller", seed=6)
sage: P == Psmaller
False

vs

sage: imf = libgap.function_factory('ImfMatrixGroup')
sage: GG = imf( 12, 3 )
sage: G = MatrixGroup(GG.GeneratorsOfGroup())
sage: Psmaller = G.as_permutation_group(algorithm="smaller", seed=6)
sage: P = G.as_permutation_group()
sage: P == Psmaller
True

dimpase · 2023-04-28T19:05:12Z

How about replacing the iffy test with

sage: P._libgap_().GroupHomomorphismByImages(Psmaller._libgap_()).Kernel()
Group(())

antonio-rojas · 2023-07-11T18:33:41Z

I finally figured out why this happens only when using system gap: the difference is the radiroot package, which is not installed by our gap system package. Why it has this side effect, I have no idea.

Matthias Koeppe added 12 commits March 18, 2023 16:19

sage.groups.matrix_gps: Move ..._gap classes to separate ..._gap modules

1e9d096

Merge branch 't/34948/sage__coding_groups___replace_imports_from_sage…

3440ffe

…___all_for_namespace_packages' into matrix_groups_modularization

sage.groups.matrix_gps: Update imports of ..._gap classes

c95f132

sage.groups.matrix_gps: Move MatrixGroupElement_gap class to a separa…

d6d5747

…te ..._gap module

sage.groups.matrix_gps: Make a namespace package, replace imports fro…

b17ab88

…m .all

sage.groups.matrix_gps: Fix up imports

a074b13

src/doc/en/reference/groups/index.rst: Add the ..._gap modules

3f1d650

Merge branch 't/32664/add_sage_rings_abc_finitefield__deprecate_is_fi…

961fa72

…nitefield' into matrix_groups_modularization

sage.groups.matrix_gps: Mark doctests # optional

009fb6c

sage.groups.{affine_gps,matrix_gps}: Mark doctests # optional

7f41289

src/sage/matrix/matrix_space.py: Use try...except for import/isinstan…

12ad68a

…ce of is_MatrixGroup, is_ArithmeticSubgroup

sage.groups.matrix_gps: When ..._gap classes cannot be imported, fall…

fb55f02

… back to _generic

Matthias Koeppe added 7 commits March 19, 2023 10:39

Merge remote-tracking branch 'upstream/develop' into matrix_groups_mo…

66f7d76

…dularization

src/doc/en/reference/groups/index.rst: Fix up

97b5480

src/sage/groups/matrix_gps/finitely_generated_gap.py: Mark a doctest …

c362fb7

…random

Merge tag '10.0.beta7' into matrix_groups_modularization

ce07608

SageMath version 10.0.beta7, Release Date: 2023-04-01

sage.groups.{affine_gps,matrix_gps}: Use '# optional - sage.rings.fin…

5dccde4

…ite_rings' when GF is involved, add '# optional - sage.symbolic'

src/sage/groups/matrix_gps/all__sagemath_polyhedra.py: Remove (commit…

45a332d

…ted by mistake)

Merge tag '10.0.beta8' into matrix_groups_modularization

f5a2807

SageMath version 10.0.beta8, Release Date: 2023-04-06

Matthias Köppe added 2 commits April 14, 2023 10:58

Merge branch 'develop' into matrix_groups_modularization

0592931

src/sage/groups/matrix_gps: Update/add copyright notices using 'git b…

9de361d

…lame -M -C -w --date=format:%Y FILE.py | sort -k2'

sage.groups.matrix_gps: Docstring cosmetics

bea635b

kwankyu reviewed Apr 16, 2023

View reviewed changes

src/sage/groups/matrix_gps/matrix_group.py Show resolved Hide resolved

kwankyu reviewed Apr 16, 2023

View reviewed changes

src/sage/groups/matrix_gps/matrix_group_gap.py Show resolved Hide resolved

src/sage/groups/matrix_gps/matrix_group*.py: Fix up markup of 'genera…

e15c6e8

…tors'

kwankyu reviewed Apr 17, 2023

View reviewed changes

src/sage/groups/matrix_gps/orthogonal_gap.py Show resolved Hide resolved

src/sage/groups/matrix_gps/orthogonal_gap.py: Add copyright notice ba…

1024a23

…sed on 'git blame -M -C -w --date=format:%Y FILE.py | sort -k2'

kwankyu approved these changes Apr 17, 2023

View reviewed changes

kwankyu added s: positive review and removed s: needs review labels Apr 17, 2023

vbraun merged commit 55ebb79 into sagemath:develop Apr 23, 2023

Uh oh!

Conversation

mkoeppe commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📚 Description

📝 Checklist

⌛ Dependencies

Uh oh!

codecov-commenter commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kwankyu commented Apr 14, 2023

Uh oh!

mkoeppe commented Apr 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwankyu left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 17, 2023

Uh oh!

mkoeppe commented Apr 17, 2023

Uh oh!

antonio-rojas commented Apr 27, 2023

Uh oh!

kwankyu commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimpase commented Apr 28, 2023

Uh oh!

kwankyu commented Apr 28, 2023

Uh oh!

antonio-rojas commented Apr 28, 2023

Uh oh!

dimpase commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonio-rojas commented Apr 28, 2023

Uh oh!

antonio-rojas commented Apr 28, 2023

Uh oh!

dimpase commented Apr 28, 2023

Uh oh!

antonio-rojas commented Jul 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mkoeppe commented Mar 18, 2023 •

edited

Loading

codecov-commenter commented Mar 19, 2023 •

edited

Loading

kwankyu commented Apr 28, 2023 •

edited

Loading

dimpase commented Apr 28, 2023 •

edited

Loading