Skip to content

handle permutations in S_n with n > 2^16 #39999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

edgarcosta
Copy link
Member

@edgarcosta edgarcosta commented Apr 22, 2025

This resolves #39998, as the issue is gap having small and large permutations:
https://github.com/gap-system/gap/blob/f0b438db8fa5786d19975e6040076fd46b08bd85/src/permutat.h#L18-L26

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

Copy link

github-actions bot commented Apr 22, 2025

Documentation preview for this PR (built with commit 4306a45; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@edgarcosta edgarcosta marked this pull request as ready for review April 23, 2025 00:00
Copy link
Member

@saraedum saraedum left a comment

Choose a reason for hiding this comment

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

I guess the failing tests have nothing to do with these changes. If so, feel free to set this to positive review.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

LGTM except I have one question: did you look at the memory usage of your doctest? Does it require a lot of memory?

@edgarcosta
Copy link
Member Author

edgarcosta commented Apr 24, 2025

In theory it just needs to allocate an array 2**17 of int32_t, but something else is going on behind the scenes.
Nonetheless, in practice the memory usage jumps by 112 MB

>>> (310099968 - 192577536)/1024.**2
112.078125

via:

/usr/bin/time -lh ./sage -c ""                                                                                                                                                          
           192577536  maximum resident set size
                   0  average shared memory size
                   0  average unshared data size
                   0  average unshared stack size
               36503  page reclaims
                 110  page faults
                   0  swaps
                   0  block input operations
                   0  block output operations
                   0  messages sent
                   0  messages received
                   0  signals received
                  10  voluntary context switches
                 158  involuntary context switches
           116133618  instructions retired
            47630290  cycles elapsed
             1458488  peak memory footprint

vs

/usr/bin/time -lh ./sage -c "SymmetricGroup(2**17)((2**16,2**17-1))._libgap_()" 
	1.00s real		0.52s user		0.25s sys
           310099968  maximum resident set size
                   0  average shared memory size
                   0  average unshared data size
                   0  average unshared stack size
               48026  page reclaims
                 114  page faults
                   0  swaps
                   0  block input operations
                   0  block output operations
                   0  messages sent
                   0  messages received
                   0  signals received
                  13  voluntary context switches
                7762  involuntary context switches
           116675395  instructions retired
            46895391  cycles elapsed
             1491280  peak memory footprint

@dimpase
Copy link
Member

dimpase commented Apr 26, 2025

should this extreme high RAM test be marked as # long time ?

@edgarcosta
Copy link
Member Author

I added # long time (100 mb)

and tried to make the example less memory intensive, but didn't succeed on that front:

sage: from resource import *
sage: print(getrusage(RUSAGE_SELF)[2]/1024.**2)
220.046875000000
sage: SymmetricGroup(2**16+1)((2**16,2**16+1))._libgap_()
(65536,65537)
sage: print(getrusage(RUSAGE_SELF)[2]/1024.**2)
319.703125000000

@dimpase
Copy link
Member

dimpase commented Apr 27, 2025

can't you make sparse permutations in GAP? (probably not, as I vaguely recall). If not, then any test is bound to be RAM-expensive.

@edgarcosta
Copy link
Member Author

There are small and large permutations, both stored in the one line representation, see
https://github.com/gap-system/gap/blob/master/src/permutat.h

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

OK

@edgarcosta
Copy link
Member Author

Thank you

@tscrim
Copy link
Collaborator

tscrim commented Apr 28, 2025

Yea, it doesn't seem like there is a better solution. I slightly lean towards making it a # not tested, but I guess it is not that big given even fairly low-memory modern computers... Perhaps we should make a brief mention on sage-devel about this?

@edgarcosta
Copy link
Member Author

I don't think is such a big issue, I am sure there other tests that take similar amount of memory.
Note that sage at startup already uses 220 MB

@dimpase
Copy link
Member

dimpase commented Apr 28, 2025

by the way, was it tested on a 32-bit system?
It might be necessary to tag this test appropriately

@edgarcosta
Copy link
Member Author

edgarcosta commented Apr 28, 2025

I do not know, as I do not have access to such machines.

I don't think the behavior will change in 32 bits, as the short permutations use 16 bit arrays, and large ones use 32 bits arrays.

Also, gap doesn't support permutation groups larger than 2^32, but one will also encounter other problems before getting there

domain = list(range(1, domain + 1))

@dimpase
Copy link
Member

dimpase commented Apr 28, 2025

if this test crashes on 32-bit CPU, it should be tagged appropriately.
so you get one result on 64-bit, and another on 32-bit (also Sage should not crash in the latter case).

(unless I missed an announcement that we dropped 32-bit)

@edgarcosta
Copy link
Member Author

I don't see why it would crash a 32bit, we are switching between arrays of

typedef uint16_t UInt2;

to arrays of

typedef uint32_t UInt4;

https://github.com/gap-system/gap/blob/master/src/common.h#L94-L97

both types are supported in 32 bits.

I am just saying that we never need to go to larger than that

@dimpase
Copy link
Member

dimpase commented Apr 28, 2025

above you point to a line of code and say that on 32-bit one won't get there

@edgarcosta
Copy link
Member Author

I was just answering the hypothetical question. Can we handle S_n for n >= 2^32? I don't think so

@tscrim
Copy link
Collaborator

tscrim commented Apr 28, 2025

I don't think is such a big issue, I am sure there other tests that take similar amount of memory. Note that sage at startup already uses 220 MB

I would be curious if there are actually other such tests. This isn’t something we considered so explicitly before, but a policy about this might be good to have. This ended up being such a clear case.

@tscrim
Copy link
Collaborator

tscrim commented Apr 28, 2025

With regards to 32 bit, I think we will just have to wait to see what Volker says from his testing… Sorry in advance Volker if this breaks on 32 bit.

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 30, 2025
sagemathgh-39999: handle permutations in S_n with n > 2^16
    
This resolves sagemath#39998, as the issue is gap having small and large
permutations:
https://github.com/gap-system/gap/blob/f0b438db8fa5786d19975e6040076fd46
b08bd85/src/permutat.h#L18-L26

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#39999
Reported by: Edgar Costa
Reviewer(s): Dima Pasechnik, Julian Rüth, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request May 1, 2025
sagemathgh-39999: handle permutations in S_n with n > 2^16
    
This resolves sagemath#39998, as the issue is gap having small and large
permutations:
https://github.com/gap-system/gap/blob/f0b438db8fa5786d19975e6040076fd46
b08bd85/src/permutat.h#L18-L26

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#39999
Reported by: Edgar Costa
Reviewer(s): Dima Pasechnik, Julian Rüth, Travis Scrimshaw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overfllow in libgap interface
4 participants