Skip to content
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

Check properly for permutations whose degree would exceed the internal limit. Document the limit. #581

Merged
merged 1 commit into from
Feb 9, 2016

Conversation

stevelinton
Copy link
Contributor

No description provided.

@stevelinton
Copy link
Contributor Author

Addresses #575

@ChrisJefferson
Copy link
Contributor

My only comment would be are we sure we don't want to limit PERM4s to 2<<28 in both 32-bit and 64-bit. How confident are we that most code works with perms between 2<<28 and 2<<32?

@stevelinton
Copy link
Contributor Author

Confident up to 2^31-1. The only thing that would break at 2^28 would be INTOBJ_INT in 32 bit.

Above 2^31-1 there might be a signed/unsigned mixup somewhere. I'll try and do some tests on babbage.

@frankluebeck
Copy link
Member

Looks fine to me, thanks.

In the documentation I would use <M>2^{28}-1</M> instead of <M>2^{{28}}-1</M> such that the braces vanish in the text version of that help page (2^28-1 instead of 2^(28)-1), but that is a matter of taste.

@stevelinton
Copy link
Contributor Author

I have done a few basic checks for permutations of degree close to 2^32-1 and they seem to work OK. Not something we can reasonably add to the test suite yet.

@frankluebeck -- fixed the formatting

@ChrisJefferson
Copy link
Contributor

I tried poking this and I couldn't break it, also couldn't come up with anything to add to a test that wouldn't break anyone with < 4GB workspaces.

Therefore, I'll merge this patch for now.

ChrisJefferson added a commit that referenced this pull request Feb 9, 2016
Check properly for permutations whose degree would exceed the internal limit. Document the limit.
@ChrisJefferson ChrisJefferson merged commit 56c6abc into gap-system:master Feb 9, 2016
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants